mirror of
https://github.com/CyberMind-FR/secubox-deb.git
synced 2026-07-29 09:14:33 +00:00
fix(profiles): apply/rollback restore real portal route + no raw traceback on corrupt JSON
Finding 1: load_routes() only returns a set of domain names (for
portal_routed), so _cmd_apply/_cmd_rollback were feeding {} into
apply_plan/rollback_to's routes=, meaning snapshot.capture always
recorded route: None for portal modules and rollback could never
restore a stopped module's WAF route. Added observe.load_route_values()
(dict domain -> [host, port]) and wired the CLI to pass it instead.
Finding 2: json.JSONDecodeError is a ValueError, not caught by
apply_plan's/_rollback_applied's except (ActuationError, OSError)
tuples nor by cli.main()'s handlers — a corrupt routes/snapshot file
escaped as a raw traceback. Added ValueError to both tuples in
api/apply.py and a rc-2 handler in cli.main().
Co-Authored-By: Gerald KERMA <devel@cybermind.fr>
This commit is contained in:
parent
1f6caf3070
commit
c839cc537d
|
|
@ -1,162 +1,189 @@
|
|||
# Task 4 report — Toolbox API: exit-country + Tor-VPN-client + obfs4-bridge CRUD
|
||||
# Task 4 report — profiles Phase 3a: CLI `apply`/`rollback` (root-only) + version + tests
|
||||
|
||||
**Status:** DONE
|
||||
**Branch:** feature/tor-enhancement-phase1 (verified, not switched)
|
||||
**Commit:** `c848b7c6` — feat(toolbox): exit-country + Tor-VPN-client + obfs4-bridge API (validated, audited, reconcile-triggered)
|
||||
**Branch:** feat/profiles-apply-phase3a (already checked out, not switched)
|
||||
**Commit:** `c9690a52` — feat(profiles): secubox-profilectl apply/rollback (root-only, dry-run default) — 0.4.0
|
||||
|
||||
## Module found + style
|
||||
## Starting state found
|
||||
|
||||
`packages/secubox-toolbox/secubox_toolbox/api.py` (4333→4557 lines) — the
|
||||
toolbox FastAPI app, `router = APIRouter(...)` included by
|
||||
`secubox_toolbox/app.py`. Read the whole file first: essentially every route
|
||||
is `async def` (grepped — the handful of apparent non-async hits were just
|
||||
stacked `@router.get(...)` decorators above one `async def`), including the
|
||||
closest analogues, `admin_tor_on` / `admin_tor_off` (#683), which don't
|
||||
shell out synchronously — they just flip a flag via `filters.set_filters()`
|
||||
(open/write/os.replace, all fast local I/O) inside an `async def`. New
|
||||
endpoints mirror that exactly: `async def`, no subprocess calls.
|
||||
`packages/secubox-profiles/api/cli.py` and `tests/test_cli.py` already carried
|
||||
an **uncommitted** implementation of `apply`/`rollback` in the working tree
|
||||
(from an earlier, interrupted session on this same branch) — imports,
|
||||
`_cmd_apply`, `_cmd_rollback`, subparser registration, and the brief's first
|
||||
two apply tests plus a concretized `test_apply_only_filters_plan` were
|
||||
already present, matching the brief closely. `debian/changelog` had **not**
|
||||
been bumped yet. I verified this against committed `HEAD` via `git stash`
|
||||
(HEAD's `cli.py`/`test_cli.py` are still Phase-1 read-only, with the old
|
||||
`test_apply_is_not_a_command_in_phase_1` guard test — 12 passed), so the
|
||||
delta below is real, not a no-op.
|
||||
|
||||
## Trigger mechanism mirrored (not invented)
|
||||
## Bug found and fixed (in `cli.py`, not test code)
|
||||
|
||||
`secubox-toolbox-tor.path` (systemd Path unit) watches
|
||||
`PathModified=/etc/secubox/toolbox/filters.json` and fires
|
||||
`secubox-toolbox-tor.service` → `sbin/secubox-toolbox-tor-reconcile` (root,
|
||||
oneshot). `admin_tor_on`/`off` trigger it by calling
|
||||
`filters.set_filters(...)`, which rewrites `filters.json`. My
|
||||
`_trigger_reconcile()` calls `set_filters({})` (empty patch — same
|
||||
tmp+rename/fallback write path, so it "touches" the file and fires the
|
||||
watcher) without touching `tor_mode` or any other flag. The portal never
|
||||
escalates privilege; the reconciler does all the nft/tor work as root,
|
||||
exactly as before.
|
||||
`_cmd_rollback` called the bare name `rollback_to(...)`, but `rollback_to`
|
||||
was **never imported anywhere** in `cli.py` (only `from . import apply,
|
||||
export` at the top, and `apply.apply_plan` used via attribute access in
|
||||
`_cmd_apply` for the documented monkeypatch reason). This is a guaranteed
|
||||
`NameError` on the very first real `rollback` invocation, and nothing in the
|
||||
existing test suite exercised the `rollback` subcommand at all, so it was
|
||||
invisible. Fixed by switching to `apply.rollback_to(...)` — same
|
||||
attribute-access pattern as `apply.apply_plan`, same rationale (a test that
|
||||
monkeypatches `api.apply.rollback_to` after import must be seen through the
|
||||
module attribute, not a name bound at import time).
|
||||
|
||||
**Known limitation, not introduced by this task, flagged for awareness:**
|
||||
`secubox-toolbox-tor-reconcile`'s `arm()` no-ops when the `toolbox_tor` nft
|
||||
table already exists (`table_present && log "already armed — no-op"`). So
|
||||
if Tor is already armed and an operator adds/removes an exit-country or
|
||||
VPN-client entry, the reconcile fires but is a no-op — the new selector
|
||||
won't apply until the next disarm→arm cycle. This is pre-existing reconciler
|
||||
behavior; the brief scoped this task to `api.py` + tests only, so I did not
|
||||
touch `sbin/secubox-toolbox-tor-reconcile`. Worth a follow-up issue if the
|
||||
operator flow expects live-apply while armed.
|
||||
## Test coverage added beyond the brief
|
||||
|
||||
## Endpoints added (all in `secubox_toolbox/api.py`, admin-gated via the
|
||||
existing `_require_tor_admin()` — blocks the public `kbin.*` vhost, same as
|
||||
`admin_tor_on/off`)
|
||||
The brief's Step 1 only specified `apply` tests (`test_apply_requires_root`,
|
||||
`test_apply_dry_run_default_acts_on_nothing`, and the placeholder
|
||||
`test_apply_only_filters_plan`, already concretized in the working tree with
|
||||
two native modules `x`/`y`, both observed on via a monkeypatched
|
||||
`_observe_all`, `--only x` asserted to leave `{"x"}` as the only id in the
|
||||
plan passed to a spied `apply.apply_plan`). It said nothing about testing
|
||||
`rollback`. Since the NameError bug above was only reachable through
|
||||
`rollback`, I added two tests mirroring the apply ones:
|
||||
`test_rollback_requires_root` (root gate → rc 1) and
|
||||
`test_rollback_dry_run_default_acts_on_nothing` (monkeypatches
|
||||
`cli.read_snapshot` and spies `api.apply.rollback_to`, asserting `apply=False`
|
||||
was passed and the call happened exactly once). Both would have failed
|
||||
against the pre-fix `cli.py` (NameError, not just a wrong return value).
|
||||
|
||||
- `GET/POST /exit_country` → `/etc/secubox/toolbox/tor-exit-country.txt`
|
||||
(one validated ISO cc per line, uppercased, deduped; POST replaces the
|
||||
whole list; 400 + **no partial write** on any bad code).
|
||||
- `GET /vpn/clients`, `POST /vpn/client`, `DELETE /vpn/client` →
|
||||
`/etc/secubox/toolbox/tor-vpn-clients.txt` (`kind:selector` lines,
|
||||
kind ∈ ip/cidr/mac; add/remove one at a time, dedup).
|
||||
- `GET /tor/bridges`, `POST /tor/bridge`, `DELETE /tor/bridge` →
|
||||
`/etc/secubox/toolbox/tor-bridges.txt` (each line must fullmatch
|
||||
`^Bridge obfs4 [][A-Za-z0-9:._=+/, -]+$`, else 400).
|
||||
|
||||
All three state files already existed as consumption targets in
|
||||
`sbin/secubox-toolbox-tor-reconcile` (`EXIT_CC_STATE`, `VPN_CLIENTS_STATE`)
|
||||
— I reused those exact paths (bridges is new, following the same
|
||||
`/etc/secubox/toolbox/tor-*.txt` convention) via
|
||||
`SECUBOX_TOR_*_PATH` env-overridable module constants (same override
|
||||
pattern as `filters.FILTERS_PATH`), which is also what the test suite
|
||||
monkeypatches.
|
||||
|
||||
Validators are exactly the brief's spec (`_valid_cc`, `_valid_selector`)
|
||||
plus `_valid_bridge` (added `[]` to the bridge charset since real obfs4
|
||||
lines carry `iat-mode=` etc. but never brackets in practice — kept the
|
||||
brief's given regex verbatim, no deviation).
|
||||
|
||||
State writes use the same atomic-tmp-then-in-place-fallback pattern as
|
||||
`filters.set_filters` (`/etc/secubox/toolbox` is 0750; the aggregator user
|
||||
may not be able to create a tmp file in the parent dir). No parent
|
||||
directory permissions are touched — `Path.mkdir(parents=True, exist_ok=True)`
|
||||
is a no-op when `/etc/secubox/toolbox` already exists (which it always does
|
||||
once `filters.json` has been written once).
|
||||
|
||||
## Audit trail
|
||||
|
||||
`_tor_audit(action, target, request)` appends
|
||||
`{ts RFC3339} secubox-toolbox operator={ip} action={action} target={target}`
|
||||
to `/var/log/secubox/audit.log`, mirroring the existing local pattern in
|
||||
`secubox_toolbox/escalate.py`'s `_audit()` (append-only, `mkdir(parents=True,
|
||||
exist_ok=True)` on the log's parent only, never chmod). The toolbox module
|
||||
has no JWT/user-identity plumbing on this router (auth happens upstream at
|
||||
nginx/aggregator level) — no existing "operator" identity is extracted
|
||||
anywhere else in `api.py` either, so `operator=` uses `request.client.host`
|
||||
as the best available identifier, falling back to `"admin"` if unavailable
|
||||
(e.g. under TestClient with no real peer).
|
||||
|
||||
## Tests
|
||||
|
||||
`packages/secubox-toolbox/tests/test_tor_vpn_api.py` — 12 tests: the
|
||||
brief's 2 validator tests verbatim (`_valid_cc`, `_valid_selector`) + 1 more
|
||||
for `_valid_bridge`, plus `TestClient(app)` endpoint-behavior tests (get
|
||||
empty, post replaces, reject-bad-code leaves no partial write, reject
|
||||
non-list, vpn add/dedup/remove round-trip, bad selector, bridge
|
||||
add/remove round-trip, bad bridge line, audit-line-written). All state
|
||||
paths and `_trigger_reconcile` are monkeypatched per-test via the brief's
|
||||
`_load()` fixture (extended with `TOR_BRIDGES` + `_TOR_AUDIT_LOG`).
|
||||
## Verification
|
||||
|
||||
Step 2 (must fail) — replayed by checking out committed `HEAD`'s `cli.py`
|
||||
against the new/extended `test_cli.py`:
|
||||
```
|
||||
cd packages/secubox-toolbox && python3 -m pytest tests/test_tor_vpn_api.py -q
|
||||
→ 12 passed
|
||||
$ .venv/bin/python -m pytest packages/secubox-profiles/tests/test_cli.py -q
|
||||
5 failed, 11 passed in 0.54s
|
||||
FAILED test_apply_requires_root — SystemExit (argparse: invalid choice 'apply')
|
||||
FAILED test_apply_dry_run_default_acts_on_nothing
|
||||
FAILED test_apply_only_filters_plan
|
||||
FAILED test_rollback_requires_root — SystemExit (argparse: invalid choice 'rollback')
|
||||
FAILED test_rollback_dry_run_default_acts_on_nothing — AttributeError: no attribute 'read_snapshot'
|
||||
```
|
||||
|
||||
Full suite:
|
||||
Step 4 (must pass) — current `cli.py` (with the `rollback_to` fix) + full
|
||||
test file:
|
||||
```
|
||||
cd packages/secubox-toolbox && python3 -m pytest tests/ -q
|
||||
→ 3 failed, 264 passed
|
||||
FAILED tests/test_bypass_sources.py::test_load_bypass_tagged_missing_source_skipped
|
||||
FAILED tests/test_media_stats.py::test_media_stats_shapes_donuts
|
||||
FAILED tests/test_media_stats.py::test_media_stats_fail_empty
|
||||
$ .venv/bin/python -m pytest packages/secubox-profiles/tests/test_cli.py -q
|
||||
16 passed in 0.12s
|
||||
```
|
||||
Verified these 3 are pre-existing and unrelated: stashed `api.py` (keeping
|
||||
the new test file aside) and reran — same 3 failures, 252 passed (252 + the
|
||||
12 new = 264, exact match, confirming zero regressions from this change).
|
||||
|
||||
TDD discipline: wrote the test file first with all endpoints/validators
|
||||
referenced but not yet implemented, confirmed collection-time `AttributeError`
|
||||
(red) against the pre-change `api.py`, then added the implementation and
|
||||
reran to green.
|
||||
Full module suite:
|
||||
```
|
||||
$ .venv/bin/python -m pytest packages/secubox-profiles/tests -q
|
||||
142 passed, 3 warnings in 0.66s
|
||||
```
|
||||
(warnings are pre-existing FastAPI `on_event` deprecation notices in
|
||||
`api/web.py`, unrelated to this task.)
|
||||
|
||||
AST check:
|
||||
```
|
||||
$ python3 -c "import ast; ast.parse(open('packages/secubox-profiles/api/cli.py').read()); print('cli OK')"
|
||||
cli OK
|
||||
```
|
||||
|
||||
## Invariants verified
|
||||
|
||||
- **Root-only**: `test_apply_requires_root` and `test_rollback_requires_root`
|
||||
both assert rc 1 when `_running_as_root()` is False, without touching
|
||||
anything else.
|
||||
- **Dry-run default**: `test_apply_dry_run_default_acts_on_nothing` and
|
||||
`test_rollback_dry_run_default_acts_on_nothing` spy on
|
||||
`apply.apply_plan`/`apply.rollback_to` and assert `apply=False` was passed
|
||||
when `--yes` is omitted, and that exactly one call happened (rc 0).
|
||||
- **`--only` filter**: `test_apply_only_filters_plan` builds two native
|
||||
modules (`x`, `y`), both observed on with no active profile (desired off
|
||||
for both → an unfiltered plan would stop both), and asserts the plan
|
||||
object handed to the spied `apply.apply_plan` contains only `{"x"}` when
|
||||
`--only x` is given — proving the filter is applied to the plan itself,
|
||||
before the dry-run print and before the (spied) actuation call.
|
||||
|
||||
## Files changed
|
||||
|
||||
- `packages/secubox-profiles/api/cli.py` — `rollback_to` → `apply.rollback_to`
|
||||
fix (the rest of the apply/rollback wiring was already present).
|
||||
- `packages/secubox-profiles/tests/test_cli.py` — kept the existing apply
|
||||
tests as-is, added `test_rollback_requires_root` and
|
||||
`test_rollback_dry_run_default_acts_on_nothing`.
|
||||
- `packages/secubox-profiles/debian/changelog` — prepended the 0.4.0 entry
|
||||
from the brief verbatim.
|
||||
|
||||
## Concerns
|
||||
|
||||
1. **Reconcile no-op-while-armed** (above) — pre-existing, out of this
|
||||
task's file scope, flagged for a follow-up issue rather than silently
|
||||
fixed.
|
||||
2. **Audit `operator=` is an IP, not an authenticated identity** — the
|
||||
module has no JWT layer on this router; this is consistent with the
|
||||
rest of `api.py`'s Tor-admin endpoints (none extract a user identity),
|
||||
not a regression introduced here.
|
||||
3. No board deployment performed — pure code + local unit tests, as
|
||||
instructed.
|
||||
1. No board deployment performed — pure code + local unit tests, as scoped.
|
||||
2. Boot-time reconciliation and the API/panel surfaces for apply/rollback are
|
||||
explicitly out of scope for this task (noted in the changelog entry as a
|
||||
follow-up), consistent with the plan's stated Phase 3a boundary.
|
||||
3. The `.superpowers/sdd/task-4-report.md` file this report replaces held
|
||||
unrelated content from a different task (a `secubox-cve-triage`
|
||||
product-absent-probes emitter) — overwritten per this task's explicit
|
||||
report-writing instruction; that content is preserved in git history if
|
||||
needed.
|
||||
|
||||
---
|
||||
## Final-review fix wave (2026-07-19)
|
||||
|
||||
## Review fix (2026-07-09) — reject IPv6 VPN selectors
|
||||
Two review findings fixed on `feat/profiles-apply-phase3a`:
|
||||
|
||||
**Commit:** `6b1c7625` — fix(toolbox): reject IPv6 VPN selectors (backend is v4-only) — no false-success
|
||||
1. **Route-value set/dict conflation** — `observe.load_routes()` returns a
|
||||
`set[str]` of domain names (used for `portal_routed` membership);
|
||||
`_cmd_apply`/`_cmd_rollback` were deriving `routes_map` from that same set
|
||||
via `routes if isinstance(routes, dict) else {}`, which is always `{}` on
|
||||
the real board. `snapshot.capture` therefore recorded `route: None` for
|
||||
every portal module, so `rollback` could stop+restart a portal module but
|
||||
never re-add its WAF route. Fix: added `observe.load_route_values()` (a
|
||||
new best-effort dict loader, domain -> `[host, port]`, `{}` on
|
||||
absent/corrupt file) and wired `_cmd_apply`/`_cmd_rollback` to pass
|
||||
`load_route_values()` — not the set-derived expression — as `apply_plan`'s
|
||||
/ `rollback_to`'s `routes=` kwarg. `load_routes()` itself is untouched and
|
||||
still feeds `_observe_all`.
|
||||
2. **Corrupt-JSON traceback escape** — `json.JSONDecodeError` is a
|
||||
`ValueError`, not caught by `apply_plan`'s forward-loop
|
||||
`except (ActuationError, OSError)` nor by `_rollback_applied`'s per-step
|
||||
except. Fix: added `ValueError` to both tuples in `api/apply.py`, and
|
||||
added a `except (OSError, ValueError)` handler in `cli.main()` (after the
|
||||
existing `ManifestError`/`StateError` and `ProtectedViolation`/
|
||||
`ApplyError` handlers) so a pre-flight corrupt-JSON failure (e.g.
|
||||
`snapshot.capture`) prints `erreur: ...` and returns rc 2 instead of a raw
|
||||
traceback.
|
||||
|
||||
Coordinator review found one real `api.py` bug (the other two findings —
|
||||
bridge-wiring + arm-noop reapply — are Task 8 reconcile work, not touched
|
||||
here).
|
||||
Tests added in `test_cli.py`:
|
||||
|
||||
**Bug:** `_valid_selector("ip"/"cidr", ...)` used bare
|
||||
`ipaddress.ip_address`/`ip_network`, which accept IPv6 (`2001:db8::1`,
|
||||
`2001:db8::/32`). But the backend `tor_vpn_src` nft set is `type ipv4_addr`
|
||||
and `populate_vpn_clients` silently skips non-v4 — so an operator POSTing an
|
||||
IPv6 client got `200 OK` + an audit line implying success while nothing was
|
||||
enforced (false success). Tightened the validator to IPv4-only
|
||||
(`.version == 4`), so v6 selectors now → 400 with no state write (honest,
|
||||
matches the Phase-1 v4-only backend).
|
||||
- `test_apply_passes_route_value_dict_not_set_derived_empty_dict` — spies on
|
||||
`apply.apply_plan`, monkeypatches `load_routes` (set) and
|
||||
`load_route_values` (dict) separately, asserts the `routes=` kwarg received
|
||||
is the dict with real `[host, port]` values, not `{}`.
|
||||
- `test_main_maps_valueerror_to_rc2_not_traceback` — makes `apply.apply_plan`
|
||||
raise a bare `ValueError` and asserts `cli.main()` returns rc 2.
|
||||
|
||||
Added 2 tests: `test_vpn_selector_rejects_ipv6` (validator: v6 ip/cidr →
|
||||
False) and `test_vpn_client_rejects_ipv6` (`POST /vpn/client` with v6 ip and
|
||||
v6 cidr → 400, no state file created).
|
||||
Mutation checks performed (both reverted after confirming):
|
||||
|
||||
```
|
||||
cd packages/secubox-toolbox && python3 -m pytest tests/test_tor_vpn_api.py -q
|
||||
→ 14 passed
|
||||
cd packages/secubox-toolbox && python3 -m pytest tests/ -q
|
||||
→ 3 failed, 266 passed (same 3 pre-existing failures, zero new)
|
||||
```
|
||||
1. Reverted `cli.py`'s `routes_map` to the old set-derived expression → new
|
||||
test failed with `assert {} == {'lyrion.gk2...': [...]}`. Confirms the
|
||||
test pins the fix.
|
||||
2. Removed `ValueError` from `apply_plan`'s forward `except` tuple and drove
|
||||
`apply_plan` directly (manual script, not committed) with a
|
||||
`_do_change` raising `json.JSONDecodeError` — confirmed it escapes
|
||||
uncaught (`ESCAPED as JSONDecodeError: bad: line 1 column 1 (char 0)`).
|
||||
Restored the tuple; `cli.main()`'s rc-2 handler remains the primary guard
|
||||
for the pre-flight case covered by the added unit test.
|
||||
|
||||
Full suite: `145 passed` (`.venv/bin/python -m pytest
|
||||
packages/secubox-profiles/tests -q`).
|
||||
|
||||
Files touched this wave: `packages/secubox-profiles/api/observe.py`,
|
||||
`packages/secubox-profiles/api/cli.py`, `packages/secubox-profiles/api/apply.py`,
|
||||
`packages/secubox-profiles/tests/test_cli.py`.
|
||||
|
||||
## Concerns (fix wave)
|
||||
|
||||
1. `load_route_values()` reads the fixed `ROUTES_FILE` path directly (same
|
||||
pattern as `load_routes()`) rather than accepting a `routes_path=`
|
||||
parameter from the CLI — consistent with existing conventions in
|
||||
`observe.py`, but means CLI-level route-path overrides (if ever added)
|
||||
would need to thread through both loaders symmetrically.
|
||||
2. No board deployment performed for this wave — pure code + local unit
|
||||
tests, as scoped by the review brief. The route-value round-trip
|
||||
(`apply --only <a portal module> --yes`, then `rollback --yes`) should be
|
||||
verified live before this branch merges, per the finding's own board-observed
|
||||
failure mode.
|
||||
needed.
|
||||
needed.
|
||||
|
|
|
|||
|
|
@ -75,7 +75,7 @@ def apply_plan(plan, manifests, actuals, *, run, observe, now, routes,
|
|||
_audit.record({"ts": now, "module": c.id, "action": c.action,
|
||||
"result": "ok", "reason": c.reason}, path=audit_path)
|
||||
applied.append(c)
|
||||
except (ActuationError, OSError) as exc:
|
||||
except (ActuationError, OSError, ValueError) as exc:
|
||||
_audit.record({"ts": now, "module": c.id, "action": c.action,
|
||||
"result": "fail", "error": str(exc)}, path=audit_path)
|
||||
rolled = _rollback_applied(applied, manifests, snap, run=run,
|
||||
|
|
@ -117,7 +117,7 @@ def _rollback_applied(applied, manifests, snap, *, run, observe, sleep, clock,
|
|||
else:
|
||||
_audit.record({"ts": now, "module": c.id, "action": rev.action,
|
||||
"result": "rollback-timeout"}, path=audit_path)
|
||||
except (ActuationError, OSError) as exc:
|
||||
except (ActuationError, OSError, ValueError) as exc:
|
||||
_audit.record({"ts": now, "module": c.id, "action": "rollback",
|
||||
"result": "fail", "error": str(exc)}, path=audit_path)
|
||||
return rolled
|
||||
|
|
|
|||
|
|
@ -25,7 +25,7 @@ from .audit import AUDIT_LOG
|
|||
from .diff import ProtectedViolation, plan_changes
|
||||
from .export import format_apt, format_json, format_pkglist, resolve_packages
|
||||
from .manifest import ManifestError, load_all
|
||||
from .observe import Actual, is_on, load_routes, observe, observe_all
|
||||
from .observe import Actual, is_on, load_route_values, load_routes, observe, observe_all
|
||||
from .scan import discover, write_drafts
|
||||
from .snapshot import SNAP_DIR
|
||||
from .snapshot import read as read_snapshot
|
||||
|
|
@ -137,7 +137,11 @@ def _cmd_apply(args) -> int:
|
|||
if args.only:
|
||||
keep = set(args.only)
|
||||
plan = [c for c in plan if c.id in keep]
|
||||
routes_map = routes if isinstance(routes, dict) else {}
|
||||
# load_route_values() (dict domaine -> [host, port]), PAS `routes` (le set
|
||||
# de load_routes() utilisé ci-dessus pour observer portal_routed) : le
|
||||
# snapshot doit capturer la vraie valeur de route pour qu'un rollback
|
||||
# puisse la restaurer (voir observe.load_route_values).
|
||||
routes_map = load_route_values()
|
||||
# run=apply.apply_plan (attribut du module, pas le nom importé) — même
|
||||
# raison que export._run plus haut : un monkeypatch de api.apply.apply_plan
|
||||
# après l'import ne changerait pas un nom lié via `from .apply import
|
||||
|
|
@ -173,8 +177,11 @@ def _cmd_rollback(args) -> int:
|
|||
# ci-dessus : `rollback_to` n'était même pas importé ici avant ce correctif
|
||||
# (NameError garanti au premier `rollback --yes` réel, non couvert par les
|
||||
# tests CLI actuels qui ne testent que `apply`).
|
||||
# routes= doit être le dict domaine -> [host, port] (load_route_values), pas
|
||||
# le set de load_routes() utilisé ci-dessus pour observer portal_routed —
|
||||
# sinon rollback_to ne peut jamais restaurer la route d'un module portail.
|
||||
report = apply.rollback_to(snap, manifests, actuals, run=_run, observe=observe,
|
||||
now=_now_iso(), routes=routes if isinstance(routes, dict) else {},
|
||||
now=_now_iso(), routes=load_route_values(),
|
||||
snap_root=SNAP_DIR, audit_path=AUDIT_LOG, apply=args.yes)
|
||||
print(f"rollback[{args.target}]: {report.status} — changed={report.changed}")
|
||||
return 0 if report.status in ("applied", "planned") else 2
|
||||
|
|
@ -342,6 +349,13 @@ def main(argv: list[str] | None = None) -> int:
|
|||
# ProtectedViolation ; un rc propre, jamais un traceback brut sur la board.
|
||||
print(f"refusé: {exc}", file=sys.stderr)
|
||||
return 3
|
||||
except (OSError, ValueError) as exc:
|
||||
# Route/snapshot JSON corrompu ou illisible en pré-flight (avant la
|
||||
# boucle apply/rollback, ex. snapshot.capture) : json.JSONDecodeError
|
||||
# est un ValueError, pas un OSError — sans ce handler ça remontait en
|
||||
# traceback brut sur la board au lieu d'un rc propre.
|
||||
print(f"erreur: {exc}", file=sys.stderr)
|
||||
return 2
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
|
|
|||
|
|
@ -89,6 +89,19 @@ def load_routes(path: Path = ROUTES_FILE) -> set[str] | None:
|
|||
return None
|
||||
|
||||
|
||||
def load_route_values(path: Path = ROUTES_FILE) -> dict:
|
||||
"""Le mapping COMPLET domaine → valeur ([host, port]) de haproxy-routes.json.
|
||||
Utilisé par apply/rollback pour capturer et restaurer la route d'un module
|
||||
portail (load_routes() ne renvoie QUE les noms de domaine). Best-effort :
|
||||
fichier absent/illisible/corrompu → {} (apply tourne en root, le dir 0750
|
||||
est traversable ; on ne veut pas planter l'apply pour ça)."""
|
||||
try:
|
||||
data = json.loads(Path(path).read_text(encoding="utf-8"))
|
||||
except (OSError, ValueError):
|
||||
return {}
|
||||
return data if isinstance(data, dict) else {}
|
||||
|
||||
|
||||
def _rss_kb(pid: str) -> int | None:
|
||||
if not pid or pid == "0":
|
||||
return None
|
||||
|
|
|
|||
|
|
@ -311,6 +311,75 @@ def test_scan_refuses_when_not_root(root, capsys, monkeypatch):
|
|||
assert "root" in err.lower()
|
||||
|
||||
|
||||
def test_apply_passes_route_value_dict_not_set_derived_empty_dict(tmp_path, monkeypatch):
|
||||
# Regression for the routes-set-discarded-as-{} bug (Phase 3a review
|
||||
# finding 1): load_routes() returns a set of domain NAMES (used by
|
||||
# _observe_all for portal_routed membership) — it never carried the
|
||||
# [host, port] VALUE. _cmd_apply used to derive routes_map from that same
|
||||
# set (`routes if isinstance(routes, dict) else {}`), which is always {}
|
||||
# on the real board, so snapshot.capture recorded route=None for every
|
||||
# portal module and rollback could never restore it. _cmd_apply must
|
||||
# instead pass load_route_values()'s dict (domain -> [host, port]) as
|
||||
# apply_plan's routes= kwarg.
|
||||
import api.cli as cli
|
||||
import api.apply as ap
|
||||
from api.apply import ApplyReport
|
||||
from api.observe import Actual
|
||||
|
||||
monkeypatch.setattr(cli, "_running_as_root", lambda: True)
|
||||
root = tmp_path / "etc"
|
||||
(root / "modules.d").mkdir(parents=True)
|
||||
(root / "profiles").mkdir(parents=True)
|
||||
(root / "modules.d" / "lyrion.toml").write_text(
|
||||
'id="lyrion"\ncategory="media"\nruntime="native"\nexposure="public"\n'
|
||||
'units=["secubox-lyrion.service"]\nprotected=false\n'
|
||||
'[portal]\ndomain="lyrion.gk2.secubox.in"\n')
|
||||
monkeypatch.setattr(cli, "_observe_all",
|
||||
lambda mans, routes: {"lyrion": Actual(enabled=True, active=True)})
|
||||
# load_routes() (the SET, used above only to observe portal_routed) still
|
||||
# reports the domain as routed; load_route_values() (the DICT, used for
|
||||
# apply_plan's snapshot) carries the real [host, port] — the two must not
|
||||
# be conflated.
|
||||
monkeypatch.setattr(cli, "load_routes", lambda: {"lyrion.gk2.secubox.in"})
|
||||
monkeypatch.setattr(cli, "load_route_values",
|
||||
lambda: {"lyrion.gk2.secubox.in": ["127.0.0.1", 9000]})
|
||||
|
||||
captured = {}
|
||||
|
||||
def spy(plan, *a, **k):
|
||||
captured["routes"] = k.get("routes")
|
||||
return ApplyReport(status="planned", changed=[c.id for c in plan])
|
||||
|
||||
monkeypatch.setattr(ap, "apply_plan", spy)
|
||||
rc = cli.main(["--root", str(root), "apply"]) # dry-run, no --yes needed to reach apply_plan
|
||||
assert rc == 0
|
||||
assert captured["routes"] == {"lyrion.gk2.secubox.in": ["127.0.0.1", 9000]}
|
||||
|
||||
|
||||
def test_main_maps_valueerror_to_rc2_not_traceback(tmp_path, monkeypatch):
|
||||
# Phase 3a review finding 2: json.JSONDecodeError is a ValueError, not an
|
||||
# OSError — a corrupt routes/snapshot JSON hit during the apply pipeline
|
||||
# (e.g. snapshot.capture's json.loads) must surface as a clean rc 2 with
|
||||
# a stderr message, never an uncaught traceback on the board.
|
||||
import api.cli as cli
|
||||
import api.apply as apply_mod
|
||||
monkeypatch.setattr(cli, "_running_as_root", lambda: True)
|
||||
root = tmp_path / "etc"
|
||||
(root / "modules.d").mkdir(parents=True)
|
||||
(root / "profiles").mkdir(parents=True)
|
||||
(root / "modules.d" / "x.toml").write_text(
|
||||
'id="x"\ncategory="infra"\nruntime="native"\nexposure="lan"\n'
|
||||
'units=["x.service"]\nprotected=false\n')
|
||||
from api.observe import Actual
|
||||
monkeypatch.setattr(cli, "_observe_all",
|
||||
lambda mans, routes: {"x": Actual(enabled=True, active=True)})
|
||||
|
||||
def boom(*a, **k):
|
||||
raise ValueError("Expecting value: line 1 column 1 (char 0)")
|
||||
monkeypatch.setattr(apply_mod, "apply_plan", boom)
|
||||
assert cli.main(["--root", str(root), "apply", "--yes"]) == 2
|
||||
|
||||
|
||||
def test_apply_error_maps_to_rc3_not_traceback(tmp_path, monkeypatch):
|
||||
# apply_plan's belt-and-suspenders ApplyError (STOP of a protected module)
|
||||
# must surface as a clean rc 3, never an uncaught traceback on the board.
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user