PR #702 - preloopdev/preloop

What & why

An automated pentest of the debug-session feature against a0409afd (isolated engine + host runner on a second machine, credentials minted exactly as the engine mints them) found four defects in the /api/v1/debug/... surface; three independent reviews then found four more issues in the first round of fixes. This PR closes all of them.

The debug surface holds a job open so an operator (or an agent) can attach to a failed step and decide retry / continue / abort. It is reachable by runners, workers, and the operator, and its machine field is worker-asserted — the four findings are respectively a state-machine hole, a worker/job hang, a debug-channel hijack, and a worker-controlled host path / argv.

Protocol surface

I confirm this change touches the runner protocol interface: NO

Everything here is preloop's own surface — /api/v1/debug/... and POST /api/v1/runs/{run_id}/debug — never /_apis/..., broker/NDJSON shapes, Twirp payloads, or check-run/OAuth wire behaviour. require_runner_bearer only adds a request extensions entry inside the engine (RunnerIdentity) and does not alter what is sent or accepted on the wire.

What changed

A finished debug session accepts no further controller operations (debug_sessions.rs). session_operation now rejects a session that is no longer open (resumed/aborted/abandoned) after the idempotency replay, so a retained finished record can no longer be flipped back to retrying: the rejection is not recorded (its request id is reusable), a previously successful request id still replays, and the session set the reaper and list read cannot gain an entry through this path.
A closed session ends the worker's verdict poll (debug_sessions.rs). take_verdict answers None (→ 404, which the worker reads as "resume the job") for a terminal session with no outstanding verdict, instead of holding the long poll until its 25 s deadline on a verdict that can never arrive — the pre-fix behaviour pinned the job. Review correction on top: an issued continue/abort is now redelivered on every poll until the worker reports close, so a lost HTTP response cannot silently turn a decision into "no decision"; retry remains consume-once (replaying it would re-run steps), and close as well as a re-opened session clear any retained verdict.
DAP port registration is bound to the runner that serves the job (debug.rs, auth.rs, openapi.rs). require_runner_bearer now records the verified runner id and register_dap_port requires it to equal the owner of the job's attempt (403 otherwise). Review correction on top: the registration is replaced when it is for the same job (any owner — so a reclaim/hand-off can repair the port) or when the registered job is no longer in progress; only a registration for a different job that is still in progress is refused (409). Before, any registered runner could point the run's DAP proxy at a port of its choosing and read/answer the operator's debug traffic.
A worker-supplied machine name is validated before any path or argv use (debug_sessions.rs, CLI debug_session.rs + main.rs, shared predicate in preloop-gha-protocol). The server drops a name that fails plausible_machine_name (ASCII letters/digits/-/_/., starting alnum, no .., ≤ 63 bytes) at the session door and logs it Debug-escaped; the CLI refuses it before the attach-marker path join and before every guest command (exec, upload, shell, resume) — including the review's : case and the previously unguarded resume path and stale-marker listing. One shared definition in preloop_gha_protocol::debug_session, used by both crates, with a drift test against preloop_vm::MachineName.

Docs: docs/contributing/debug-sessions-design.md (terminal-state and verdict-delivery semantics, machine-name rule and its residual), docs/control-plane/guides/debugging.mdx (verdict/new-pause semantics, DAP registration policy), docs/control-plane/guides/agents.mdx. Changelog under [Unreleased] → ### Security.

Honest limits

The DAP fix binds listen-token registrations only. The real worker's own registration does not work today for independent, pre-existing reasons (its job-message credential is a runtime token — sub = preloop-job-… — not a runner listen token, and it appends the wrong URL prefix and the agent-job UUID); and the legacy DAP proxy still falls back to the fixed engine-local 127.0.0.1:4711 when nothing is registered. Both stay open (design-doc open items; the fallback is L3). So this closes the hijack for callers who can present a runner listen token — a machine credential issued by the control plane — not "any caller".
The machine-name rule is shape, not identity. A worker can still assert a different, valid machine name that is not bound to its own job; binding the name to the run's runner at open is a follow-up (stated in the design doc and changelog).
retry over a lost response can still strand a session until the 90 s worker-liveness reaper retires it: redelivering a retry would re-run steps, so it is consumed once by design.
Not fixed here (documented open items from the same pentest): L1 lease takeover by name, L2 audit attribution is caller-declared, L3 the 4711 fallback above, L4 restart loses pause credit, L5 workflow-controlled strings are printed by the CLI; and the untested hypotheses H1/H2/H4/H5.

Verification performed

Run on macOS (Apple silicon) from a clean checkout of a0409afd with this PR's diff applied:

cargo clippy --locked --workspace --all-targets -- -D warnings          # clean (CI's recipe)
cargo clippy --locked -p preloop-cli -p preloop-gha-protocol --all-targets -- -D warnings   # clean
cargo test --locked -p preloop-runner-server --features test-support --lib debug_sessions::  # 28 passed
cargo test --locked -p preloop-runner-server --features test-support --test registration     # 46 passed
cargo test --locked -p preloop-runner-server --features test-support --test runs_api --test store_env   # 59 + 4 passed
cargo test --locked -p preloop-cli --bin preloop                                            # 315 passed
cargo test --locked -p preloop-gha-protocol                                                 # 104 + 2 passed
cargo test --locked -p preloop-runner --lib debug_pause                                     # 22 passed
rustfmt --edition 2024 --check <changed files>                          # clean

Every behavioural test below was also run against the unfixed production code (A = base a0409afd) and against the first round of fixes (B = the same branch before the review corrections), with the test modules kept identical so the only variable is the production code:

Test	A (no fix)	B (first draft)
lib `a_machine_name_that_is_not_a_name_is_refused`	`../../../../.zshrc` survived the door	`a:b` survived the door
lib `a_terminal_session_refuses_operations_and_stops_the_poll`	retry on the finished session accepted	(passed)
lib `a_terminal_verdict_is_redelivered_until_the_worker_closes`	verdict dropped after one delivery	verdict consumed by the first pickup
integ `a_finished_debug_session_rejects_further_operations`	left 200, right 400	(passed)
integ `a_closed_debug_session_ends_the_verdict_poll`	left 200, right 404	(passed)
integ `a_debug_verdict_is_redelivered_until_the_worker_closes`	second poll had no verdict	second poll 404 `no such session`
integ `debug_dap_registration_binds_to_the_current_job_owner`	foreign registration 200 instead of 403	hand-off re-registration 409 instead of 200
cli `claim_refuses_a_hostile_machine_name_before_touching_the_home`	guard absent	(passed)
cli `guest_helpers_refuse_a_hostile_machine_name`	guard absent	`guest_resume_command` still unguarded
cli `a_hostile_marker_file_is_never_offered_as_a_machine`	hostile stale marker offered	hostile stale marker offered (`find_debug_machine` unfiltered)
proto `machine_name_rule_accepts_and_refuses_the_right_names`	new function (no pre-fix state)	new function (no pre-fix state)

a_pending_verdict_survives_the_terminal_state_it_implies, a_retry_verdict_is_consumed_by_its_first_pickup and a_reopened_session_does_not_inherit_the_previous_verdict pass before and after: they exist to catch an over-broad terminal rule and verdict bleed between pauses.

Pre-existing base conditions (not introduced here)

cargo fmt --all --check on the base commit already flags crates/preloop-cli/src/init.rs:4243.
cargo clippy -p preloop-runner-server --all-targets --features test-support -- -D warnings fails on test targets this PR does not touch (tests/mutation_targets.rs:598, tests/recovery.rs:2552, collapsible_if). CI does not run clippy with that feature, and the repo's just clippy recipe passes.

Required gates

just test-ci passes locally — not run end to end here (runner-watch conformance and supply-chain gates are CI-side); fmt/clippy/tests were run per touched crate as above. cargo clippy --locked --workspace --all-targets -- -D warnings is green.
Supply-chain isolation: no dependency or policy file is touched (no Cargo.toml/Cargo.lock change)
N/A — no protocol/wire shape changed
N/A — no property-tested subsystem changed
New wire fields/events: none
No secrets, credentials, or deployment-specific paths introduced

Checklist

Tests added/updated for the new observable contract (unit + integration; the integration tests drive the real HTTP routes, real runner registration/claim, and the real liveness/reap path)
Docs updated (docs/contributing/debug-sessions-design.md, docs/control-plane/guides/debugging.mdx, docs/control-plane/guides/agents.mdx)
Changelog entry added ([Unreleased] → ### Security)

Summary by cubic

Closes four pentest findings in the debug-session surface and fixes a flaky server test.

Operations against a resumed, aborted, or abandoned session are rejected with 400; rejections aren't recorded, and successful request IDs still replay.
A closed session answers the verdict poll with 404 so the held job resumes. continue, abort, and retry are redelivered on every poll until the worker closes the session, so a lost response can't turn a decision into "no decision" or strand a worker polling a session that will never answer.
DAP port registration requires the caller to be the runner serving the job (403 otherwise); same-job replacements are allowed, and a registration for a different live job is refused with 409. Ownership and status are re-checked under a per-run lock that also serializes concurrency without parking the global engine lock, closing the handoff race and avoiding stalls on backend round trips.
Worker-supplied machine names must match plausible_machine_name (ASCII alnum, -, _, ., ≤63 bytes, no ..); the server drops them at session open and the CLI refuses them before any path join or guest-tool argument.
action_download_info_returns_batch_download_collection retries a transient ref-to-SHA lookup once after clearing the negative cache.

The runner protocol surface is unchanged.

Summary by CodeRabbit

Security
- Worker-provided machine names are validated before use; invalid names are rejected.
- DAP port registration is limited to the runner currently owning the job. Conflicting registrations for another active job are rejected, while same-job and stale registrations can be replaced.
Bug Fixes
- Finished debug sessions reject new operations while replaying responses to previously successful requests.
- Unacknowledged verdicts are redelivered until the worker closes the session; closing ends verdict polling.
Documentation
- Updated debugging guidance to cover verdict delivery, finished-session behavior, and DAP registration rules.