From 5fecc5bb637c552b2f574ae91052cbf46c7ae3f0 Mon Sep 17 00:00:00 2001 From: Tran Duc Anh Dang Date: Fri, 13 Nov 2026 02:43:29 +1100 Subject: [PATCH 1/2] feat(crew): approve once needs the person's click in the desktop app The core half of local presence: the shell describes a hold and grants a one-time nonce over the parent channel, PresenceBook keeps one grant per hold for 60 seconds, and approve-once takes it instead of the account's confirmation. Nothing mints a grant until the shell dialog (next PR) exists. --- crates/orbyn-core/src/account/flow/confirm.rs | 11 +- crates/orbyn-core/src/app.rs | 3 + .../orbyn-core/src/crew/autopilot/README.md | 1 + crates/orbyn-core/src/crew/autopilot/held.rs | 98 ++--- crates/orbyn-core/src/crew/autopilot/mod.rs | 3 +- .../orbyn-core/src/crew/autopilot/presence.rs | 191 +++++++++ crates/orbyn-core/src/crew/mod.rs | 10 +- .../src/crew/tests/autopilot_held.rs | 394 +++++++++++++----- crates/orbyn-core/src/crew/tests/harness.rs | 3 - crates/orbyn-core/src/error.rs | 15 +- crates/orbyn-core/src/host/README.md | 1 + crates/orbyn-core/src/host/mod.rs | 1 + crates/orbyn-core/src/host/presence.rs | 208 +++++++++ crates/orbyn-core/src/host/shell.rs | 7 +- crates/orbyn-core/src/util.rs | 10 + crates/orbynd/src/http/access.rs | 4 +- crates/orbynd/src/main.rs | 2 +- crates/orbynd/src/routes/crew.rs | 27 +- crates/orbynd/src/routes/crew_held_tests.rs | 214 ++++++---- crates/orbynd/src/routes/crew_tests.rs | 8 +- crates/orbynd/tests/presence_stdio.rs | 121 ++++++ docs/architecture.md | 16 + docs/crew.md | 35 +- 23 files changed, 1093 insertions(+), 290 deletions(-) create mode 100644 crates/orbyn-core/src/crew/autopilot/presence.rs create mode 100644 crates/orbyn-core/src/host/presence.rs create mode 100644 crates/orbynd/tests/presence_stdio.rs diff --git a/crates/orbyn-core/src/account/flow/confirm.rs b/crates/orbyn-core/src/account/flow/confirm.rs index 89a47863..fae78895 100644 --- a/crates/orbyn-core/src/account/flow/confirm.rs +++ b/crates/orbyn-core/src/account/flow/confirm.rs @@ -8,6 +8,7 @@ use super::super::AccountCenter; use super::super::api::Password; use super::{CONFIRM_WINDOW, ConfirmFor, Confirmed}; use crate::error::{CoreError, Result}; +use crate::util::same_secret; /// A confirmation waiting to be used. In memory only, never stored. pub(in crate::account) struct Grant { @@ -17,16 +18,6 @@ pub(in crate::account) struct Grant { pub(super) nonce: String, } -/// Compares two secrets without stopping at the first difference. -fn same_secret(left: &str, right: &str) -> bool { - left.len() == right.len() - && left - .bytes() - .zip(right.bytes()) - .fold(0u8, |diff, (a, b)| diff | (a ^ b)) - == 0 -} - impl AccountCenter { // Confirm it's you diff --git a/crates/orbyn-core/src/app.rs b/crates/orbyn-core/src/app.rs index 5b911b6f..db5f6995 100644 --- a/crates/orbyn-core/src/app.rs +++ b/crates/orbyn-core/src/app.rs @@ -132,6 +132,8 @@ pub struct Core { pub(crate) setups: Arc, pub updates: Arc, pub shell: ShellBridge, + /// A person's approvals given in the desktop app, in memory only. + pub(crate) presence: crate::host::presence::PresenceBook, pub usage: Arc, pub clones: ClonePool, pub previews: PreviewManager, @@ -276,6 +278,7 @@ impl Core { setups, updates, shell: ShellBridge::default(), + presence: crate::host::presence::PresenceBook::default(), usage, clones: ClonePool::default(), previews: PreviewManager::default(), diff --git a/crates/orbyn-core/src/crew/autopilot/README.md b/crates/orbyn-core/src/crew/autopilot/README.md index 5159175f..ca550347 100644 --- a/crates/orbyn-core/src/crew/autopilot/README.md +++ b/crates/orbyn-core/src/crew/autopilot/README.md @@ -16,6 +16,7 @@ The second half of the slice is the merge queue, the after-merge Review queue an | `tripwire` | 3 denials in a row, 20 in a run, and the injection detector | | `ledger` | Writes the approval ledger rows: redacted, the key's text withheld, scanned | | `held` | A command the chain denied, held in memory for a person: approve once, deny, dismiss. No agent approves; a person may approve once | +| `presence` | The shell messages that carry a person's approval of a hold (`presence_describe`, `presence_grant`) and what they check | | `review` | `Core`'s side: the mode, the approvers, and `ActionReview` for the gate | | `merge/risk` | The risk tier of a diff, with its reasons. Pure | | `merge/holds`, `merge/guard` | What holds a merge: the person-only diffs, the owed lanes, the mode now | diff --git a/crates/orbyn-core/src/crew/autopilot/held.rs b/crates/orbyn-core/src/crew/autopilot/held.rs index c2614b83..9ce0a245 100644 --- a/crates/orbyn-core/src/crew/autopilot/held.rs +++ b/crates/orbyn-core/src/crew/autopilot/held.rs @@ -11,8 +11,10 @@ //! redacted command and a hash of the exact one. Each hold is decided once, under one lock, so a racing approve and deny have //! exactly one winner. //! -//! Approving is the only answer that needs proof that a person is present: the account's "Confirm it's you" (the password again, -//! one-time nonce in `x-orbyn-confirm`, bound to this hold's id). See [`CONFIRM_PREFIX`] for what it does and does not protect. +//! Approving is the only answer that needs proof that a person is present: a grant the desktop app records after the person +//! clicks Approve in its own native dialog (see [`presence`](super::presence) and docs/crew.md, "Approving once: local presence"). +//! The page sends that grant's nonce in `x-orbyn-confirm`; a caller that only speaks HTTP to the service has none. This is not +//! OS-level presence: a process of the same OS user can still drive the dialog or the shell. use std::sync::{Mutex, MutexGuard, PoisonError}; @@ -22,12 +24,12 @@ use serde_json::{Value, json}; use super::hard::{self, Scene}; use super::ledger; -use crate::account::ConfirmFor; use crate::app::Core; use crate::crew::lifecycle::is_final; use crate::crew::{key_needles, wire_name}; use crate::error::{CoreError, Result}; use crate::governance::approvals::ActionAsk; +use crate::host::presence::PresenceFor; use crate::store::{CrewRun, CrewTask}; use crate::util::{new_id, sha256_hex, unix_now}; @@ -43,22 +45,6 @@ const OPEN_PER_TASK: usize = 20; /// Holds kept in all, open or decided; the oldest decided ones go first. const KEPT: usize = 200; -/// The prefix of the id a hold's confirmation is made for. The window asks `POST /api/account/confirm` with the action -/// `approve_device` and the `device_id` `crew-approve-once:`, and sends the nonce it gets back as `x-orbyn-confirm`. -/// -/// **What it protects:** the one-time nonce comes only from a password check, so a caller that holds nothing but the service -/// token (a worker, per #441) cannot approve; it is for one hold, used up by its first use, ten minutes at most, in memory only. -/// **What it does not:** it needs a signed-in account (without one, nothing can be approved once), and a worker that can read -/// the password or drive the window (the sandbox, #441) can still defeat it. Deny and dismiss need no password, so a caller -/// with the token can deny or dismiss a hold (never approve one). -/// -/// **In release builds approve-once always fails** (`account_disabled`: the account is off). **In dev builds the stand-in -/// account's password is public**, so a worker holding the token (#441) could call `/api/account/confirm` itself, by a script N1 -/// does not read, and approve its own hold: the protection is only as strong as the account password. As a code guard, -/// approve-once is refused while a seeded stand-in account (an `@example.com` address on the development backend) is signed in. -/// A dedicated confirmation action replaces this id convention once the account's own code can change. -pub const CONFIRM_PREFIX: &str = "crew-approve-once:"; - #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(super) enum Stage { Held, @@ -360,11 +346,11 @@ pub(crate) enum Decision { } impl Core { - fn held_book(&self) -> MutexGuard<'_, HeldBook> { + pub(super) fn held_book(&self) -> MutexGuard<'_, HeldBook> { lock(&self.crew_autopilot.held) } - fn held_needles(&self, task_id: &str) -> Vec { + pub(super) fn held_needles(&self, task_id: &str) -> Vec { self.store .crew_answer_key(task_id) .ok() @@ -507,7 +493,7 @@ impl Core { } /// Ends what ran out of time, and every open hold or approval whose worker's task is over. - fn held_sweep(&self, now: i64) { + pub(super) fn held_sweep(&self, now: i64) { let ended = self.held_book().sweep(now); self.held_ended(ended); let open: Vec<(String, String)> = self @@ -524,7 +510,7 @@ impl Core { } } - fn held_ended_by_task(&self, id: &str) { + pub(super) fn held_ended_by_task(&self, id: &str) { let closed = { let mut book = self.held_book(); let Some(item) = book.items.iter_mut().find(|i| i.id == id) else { @@ -541,7 +527,7 @@ impl Core { } /// The task and run of a worker thread that is still running a task. - fn live_worker(&self, thread_id: &str) -> Option<(CrewTask, CrewRun)> { + pub(super) fn live_worker(&self, thread_id: &str) -> Option<(CrewTask, CrewRun)> { let (task, run) = self.store.crew_task_for_worker(thread_id).ok().flatten()?; (!is_final(&task.state)).then_some((task, run)) } @@ -559,10 +545,11 @@ impl Core { } } - /// Approves one held command, once. Only a person's action: the call needs the nonce that "Confirm it's you" made for this - /// hold (see [`CONFIRM_PREFIX`]), and nothing else in the core calls it. It checks, in this order: the confirmation (without - /// using it), that the hold is open, that its task is still running, that the command can be read whole, and the - /// always-denied list again, now; then uses the confirmation up and decides, once. + /// Approves one held command, once. Only a person's action: the call needs the nonce the desktop app recorded when the + /// person clicked Approve in its own dialog for this hold, as the hold stands now (see [`PresenceFor`]), and nothing else in + /// the core calls it. It checks, in this order: that a desktop shell exists, the grant (without using it), that the hold is + /// open, that its task is still running, that the command can be read whole, and the always-denied list again, now; then uses + /// the grant up and decides, once. A refusal at any step leaves the grant as it was. pub async fn autopilot_approve_once( &self, id: &str, @@ -579,20 +566,24 @@ impl Core { ) -> Result { self.crew_preview_only()?; super::merge::valid_id(id)?; - let action = ConfirmFor::ApproveDevice(format!("{CONFIRM_PREFIX}{id}")); - if !self.account.available() { - return Err(CoreError::AccountDisabled); - } - if self.stand_in_account_signed_in().await { - return Err(CoreError::PolicyDenied( - "Approving once is not available while the development stand-in account is signed in: its password is public." - .into(), - )); - } - if !self.account.confirmed_for(&action, nonce) { - return Err(CoreError::ReauthRequired); + if !self.shell.attached() { + return Err(CoreError::DesktopRequired); } self.held_sweep(now); + // A hold that is not there has no grant, so it answers as a missing grant does. + let seen = self.held_book().get(id).cloned(); + let want = |held: Option<&Held>| PresenceFor::CrewApproveOnce { + hold: id.to_owned(), + thread: held.map(|h| h.thread_id.clone()).unwrap_or_default(), + task: held.map(|h| h.task_id.clone()).unwrap_or_default(), + command_sha256: held.map(|h| h.bound.clone()).unwrap_or_default(), + }; + if !self + .presence + .confirmed_for(&want(seen.as_ref()), nonce, now) + { + return Err(CoreError::PresenceRequired); + } let held = self.held_book().open(id).map_err(Self::held_failure)?; let Some((task, run)) = self .live_worker(&held.thread_id) @@ -620,37 +611,20 @@ impl Core { refusal.0, refusal.1 ))); } - // The confirmation is used up here, then the one decision is made: a racing deny or a second approve loses. - self.account.require_confirmed(action, nonce).await?; + // The grant is used up here, then the one decision is made: a racing deny or a second approve loses. + if !self.presence.take(&want(Some(&held)), nonce, now) { + return Err(CoreError::PresenceRequired); + } let decided = self.held_book().decide(id, now, Stage::Approved); let held = decided.map_err(Self::held_failure)?; self.held_row( &held, "approved_once", - &json!({ "via": "local_session", "expires_in_secs": APPROVAL_LIFETIME_SECS }), + &json!({ "via": "desktop_presence", "expires_in_secs": APPROVAL_LIFETIME_SECS }), ); Ok(HeldBook::view(&held, now)) } - /// The development backend, signed in as one of its seeded `@example.com` accounts, whose password is public. - async fn stand_in_account_signed_in(&self) -> bool { - #[cfg(any(test, debug_assertions))] - { - if self.config.account_backend != crate::account::AccountBackendKind::Development { - return false; - } - self.account - .view(std::env::consts::OS) - .await - .session - .is_some_and(|session| session.email.to_ascii_lowercase().ends_with("@example.com")) - } - #[cfg(not(any(test, debug_assertions)))] - { - false - } - } - /// The always-denied list, against the exact request, as it stands now. async fn refuse_now( &self, diff --git a/crates/orbyn-core/src/crew/autopilot/mod.rs b/crates/orbyn-core/src/crew/autopilot/mod.rs index 75bed135..9ea05bcb 100644 --- a/crates/orbyn-core/src/crew/autopilot/mod.rs +++ b/crates/orbyn-core/src/crew/autopilot/mod.rs @@ -8,6 +8,7 @@ mod held; mod hosts; mod ledger; mod merge; +mod presence; mod review; mod shell; mod tier; @@ -15,7 +16,7 @@ mod tripwire; pub use chain::{Answer, Approver, Approvers, Outcome, Summary}; pub use hard::{RULES, Refusal, Scene, refuse}; -pub use held::{APPROVAL_LIFETIME_SECS, CONFIRM_PREFIX, HELD_LIFETIME_SECS, HeldBook, SHOWN_MAX}; +pub use held::{APPROVAL_LIFETIME_SECS, HELD_LIFETIME_SECS, HeldBook, SHOWN_MAX}; pub use hosts::read_allowed_hosts; pub use merge::{ Answering, COMMENT_MAX, DIGEST_HOURS_MAX, DiffFacts, MergeOutcome, MergeTarget, OFFSET_MAX, diff --git a/crates/orbyn-core/src/crew/autopilot/presence.rs b/crates/orbyn-core/src/crew/autopilot/presence.rs new file mode 100644 index 00000000..1a6dca2d --- /dev/null +++ b/crates/orbyn-core/src/crew/autopilot/presence.rs @@ -0,0 +1,191 @@ +//! The service's half of approving a held command in the desktop app. **Nothing here approves.** It answers what the shell asks +//! on the channel only the shell holds and records the person's grant; the approval itself is `held`'s, and needs the grant. +//! +//! The messages are newline JSON, as the rest of that channel (`host::shell`): +//! +//! | The shell sends | The service answers on stdout | +//! | --- | --- | +//! | `presence_describe {request_id, hold_id}` | `presence_hold {request_id, hold_id, thread_id, task_id, command_shown, command_sha256, worker_name, task_title}` or `presence_refused {request_id, code}` | +//! | `presence_grant {request_id, nonce, hold_id, thread_id, task_id, command_sha256}` | `presence_ready {request_id}` or `presence_refused {request_id, code}` | +//! +//! The command, the worker's name and the task's title all come from here, redacted and bounded, never from the window: the +//! dialog shows what the service says the hold is. `command_sha256` is the hold's binding hash (the task, the thread, the kind, +//! the whole command and its details). A grant is recorded only if the hold is open now and every field matches it as it stands +//! now; the shell hands the nonce to the page only after `presence_ready`. +//! +//! Refusal codes: `unavailable` (the Preview flag is off or no shell is attached), `invalid` (a field is malformed), `not_found`, +//! `gone` (decided, expired or its task ended), `unreadable` (too long to read whole), `mismatch` (a bound field changed). + +use serde_json::{Value, json}; + +use crate::host::presence::{PresenceFor, is_nonce}; +use crate::host::shell::is_id; +use crate::util::unix_now; + +use super::held::{Failure, Stage}; +use crate::app::Core; + +/// The longest worker name and task title the dialog shows, in characters. +pub const WORKER_NAME_MAX: usize = 80; +pub const TASK_TITLE_MAX: usize = 120; + +/// What the dialog shows about a hold, as the service has it now. +struct Facts { + hold: String, + thread: String, + task: String, + shown: String, + bound: String, + worker: String, + title: String, +} + +/// Text for the dialog: redacted, the answer key's text withheld, cut to `max` characters, and empty if a secret or the key's +/// text would still show. +fn bounded(text: &str, max: usize, needles: &[String]) -> String { + let clean = crate::crew::withhold(&crate::crew::redact(text), needles); + let cut: String = clean.chars().take(max).collect(); + let value = json!(cut); + if crate::crew::refuse_secret(&value).is_ok() + && crate::crew::refuse_leak(&value, needles).is_ok() + { + cut + } else { + String::new() + } +} + +impl Core { + /// Takes a line meant for this side. False leaves it for the next reader. + pub fn presence_line(&self, line: &str) -> bool { + self.presence_line_at(line, unix_now()) + } + + pub(crate) fn presence_line_at(&self, line: &str, now: i64) -> bool { + let Ok(message) = serde_json::from_str::(line) else { + return false; + }; + let text = |key: &str| message.get(key).and_then(Value::as_str).unwrap_or_default(); + let kind = text("type"); + if kind != "presence_describe" && kind != "presence_grant" { + return false; + } + let request = text("request_id"); + if !is_id(request) { + tracing::warn!("ignoring a presence message without a usable request id"); + return true; + } + let answer = if kind == "presence_describe" { + self.presence_describe(request, text("hold_id"), now) + } else { + self.presence_grant(request, &message, now) + }; + let answer = answer.unwrap_or_else( + |code| json!({ "type": "presence_refused", "request_id": request, "code": code }), + ); + if !self.shell.send(answer) { + tracing::warn!("a presence answer found no shell to go to"); + } + true + } + + fn presence_describe( + &self, + request: &str, + hold: &str, + now: i64, + ) -> Result { + let facts = self.presence_facts(hold, now)?; + Ok(json!({ + "type": "presence_hold", + "request_id": request, + "hold_id": facts.hold, + "thread_id": facts.thread, + "task_id": facts.task, + "command_shown": facts.shown, + "command_sha256": facts.bound, + "worker_name": facts.worker, + "task_title": facts.title, + })) + } + + fn presence_grant( + &self, + request: &str, + message: &Value, + now: i64, + ) -> Result { + let text = |key: &str| message.get(key).and_then(Value::as_str).unwrap_or_default(); + if !self.shell.attached() { + return Err("unavailable"); + } + let nonce = text("nonce"); + if !is_nonce(nonce) { + return Err("invalid"); + } + let facts = self.presence_facts(text("hold_id"), now)?; + if text("thread_id") != facts.thread + || text("task_id") != facts.task + || text("command_sha256") != facts.bound + { + return Err("mismatch"); + } + self.presence.put( + PresenceFor::CrewApproveOnce { + hold: facts.hold, + thread: facts.thread, + task: facts.task, + command_sha256: facts.bound, + }, + nonce.to_owned(), + now, + ); + Ok(json!({ "type": "presence_ready", "request_id": request })) + } + + /// The hold as it stands now, if a person may still approve it. + fn presence_facts(&self, hold: &str, now: i64) -> Result { + if self.crew_preview_only().is_err() || !self.shell.attached() { + return Err("unavailable"); + } + if super::merge::valid_id(hold).is_err() { + return Err("invalid"); + } + self.held_sweep(now); + let held = self + .held_book() + .open(hold) + .map_err(|failure| match failure { + Failure::Unknown => "not_found", + Failure::Over | Failure::Decided(_) => "gone", + })?; + debug_assert_eq!(held.stage, Stage::Held); + let Some((_, run)) = self + .live_worker(&held.thread_id) + .filter(|(task, _)| task.id == held.task_id) + else { + self.held_ended_by_task(hold); + return Err("gone"); + }; + if !held.readable { + return Err("unreadable"); + } + let needles = self.held_needles(&held.task_id); + let title = self + .store + .crew_brief(&held.task_id) + .ok() + .flatten() + .map(|brief| brief.outcome) + .unwrap_or_default(); + Ok(Facts { + hold: held.id.clone(), + thread: held.thread_id.clone(), + task: held.task_id.clone(), + shown: held.shown.clone(), + bound: held.bound.clone(), + worker: bounded(&run.member, WORKER_NAME_MAX, &needles), + title: bounded(&title, TASK_TITLE_MAX, &needles), + }) + } +} diff --git a/crates/orbyn-core/src/crew/mod.rs b/crates/orbyn-core/src/crew/mod.rs index 692005c5..fec54a19 100644 --- a/crates/orbyn-core/src/crew/mod.rs +++ b/crates/orbyn-core/src/crew/mod.rs @@ -37,11 +37,11 @@ mod tests; pub(crate) use autopilot::GateReview; pub use autopilot::{ APPROVAL_LIFETIME_SECS, Answer, Answering, Approver, Approvers, Autopilot, COMMENT_MAX, - CONFIRM_PREFIX, DIGEST_HOURS_MAX, DiffFacts, Grants, HELD_LIFETIME_SECS, IN_A_ROW, IN_A_RUN, - MergeOutcome, MergeTarget, Mode, ModeScope, Need, OFFSET_MAX, Outcome, PAGE_DEFAULT, PAGE_MAX, - Page, RULES, Rate, RecentTotals, Refusal, ReviewAnswer, ReviewItem, Risk, SHOWN_MAX, Scene, - Scored, Summary, WatchOutcome, area_of, areas_of, check_words, injection_in, lane_ran, - owed_lanes, refuse, review_window, score, sensitive, tier_of, valid_id, + DIGEST_HOURS_MAX, DiffFacts, Grants, HELD_LIFETIME_SECS, IN_A_ROW, IN_A_RUN, MergeOutcome, + MergeTarget, Mode, ModeScope, Need, OFFSET_MAX, Outcome, PAGE_DEFAULT, PAGE_MAX, Page, RULES, + Rate, RecentTotals, Refusal, ReviewAnswer, ReviewItem, Risk, SHOWN_MAX, Scene, Scored, Summary, + WatchOutcome, area_of, areas_of, check_words, injection_in, lane_ran, owed_lanes, refuse, + review_window, score, sensitive, tier_of, valid_id, }; pub use background::{ BackgroundEnd, EVERY_HOURS_MAX, MINUTES_MAX, NAME_MAX, SETTING_MAX, TURNS_MAX, diff --git a/crates/orbyn-core/src/crew/tests/autopilot_held.rs b/crates/orbyn-core/src/crew/tests/autopilot_held.rs index bfd72538..6d8b2bad 100644 --- a/crates/orbyn-core/src/crew/tests/autopilot_held.rs +++ b/crates/orbyn-core/src/crew/tests/autopilot_held.rs @@ -3,53 +3,69 @@ //! once, denies it or dismisses it. Values that stand for a secret are built at run time and named by label. use orbyn_contracts::{ActionKind, AutopilotHeldState as State, StopReason}; +use serde_json::{Value, json}; +use tokio::sync::mpsc; use super::autopilot_support::{Scripted, approvers, ask, events, kinds, request, running, scope}; use super::harness::{KEY_MARKER, World, world}; -use crate::account::{ConfirmFor, Password}; -use crate::crew::{APPROVAL_LIFETIME_SECS, CONFIRM_PREFIX, HELD_LIFETIME_SECS, Mode, ModeScope}; +use crate::crew::{APPROVAL_LIFETIME_SECS, HELD_LIFETIME_SECS, Mode, ModeScope}; use crate::error::CoreError; use crate::governance::approvals::Verdict; +use crate::host::presence::PRESENCE_LIFETIME_SECS; use crate::testing::run_git; use crate::util::unix_now; -/// The development account's password, assembled so no literal sits in the source. -fn account_password() -> String { - ["correct", "horse", "battery"].join(" ") +/// Plays the desktop shell: sends one line the way the shell does and returns the service's answer. +fn shell_says(w: &World, line: &Value, now: i64) -> Value { + let (lines, mut answers) = mpsc::unbounded_channel(); + w.core.shell.attach(lines); + assert!(w.core.presence_line_at(&line.to_string(), now)); + answers.try_recv().expect("the service answered") } -async fn signed_in(w: &World) { - // A private account, not one of the stand-in's seeded ones, whose password is public. - w.core - .account - .start_sign_up( - "K7QM2XWH9QXA3C2D", - "crew-test@example.org", - Password::new(account_password()), - ) - .await - .unwrap(); - w.core.account.verify_email_code("ABCDEFGH").await.unwrap(); +fn describe(w: &World, hold: &str, now: i64) -> Value { + shell_says( + w, + &json!({ "type": "presence_describe", "request_id": "r-1", "hold_id": hold }), + now, + ) } -/// What the window does: "Confirm it's you" for this hold, answering with the nonce. -async fn nonce_for(w: &World, hold: &str) -> String { - w.core - .account - .confirm( - ConfirmFor::ApproveDevice(format!("{CONFIRM_PREFIX}{hold}")), - Password::new(account_password()), - ) - .await - .unwrap() +const NONCE: &str = "0123456789abcdef0123456789abcdef"; + +/// The whole of what the shell does after a click on Approve: describe, then grant what was described. Returns the nonce. +fn grant_at(w: &World, hold: &str, nonce: &str, now: i64) -> String { + let shown = describe(w, hold, now); + assert_eq!(shown["type"], "presence_hold", "{shown}"); + let ready = shell_says( + w, + &json!({ + "type": "presence_grant", "request_id": "r-2", "nonce": nonce, "hold_id": hold, + "thread_id": shown["thread_id"], "task_id": shown["task_id"], + "command_sha256": shown["command_sha256"], + }), + now, + ); + assert_eq!( + ready, + json!({ "type": "presence_ready", "request_id": "r-2" }) + ); + nonce.to_owned() +} + +fn grant_for(w: &World, hold: &str) -> String { + grant_at(w, hold, NONCE, unix_now()) } -/// A running task whose lead says no to everything, with the person signed in. +/// A running task whose lead says no to everything, with a desktop shell attached. async fn held_world() -> (World, String) { let (w, id) = running().await; let lead = Scripted::no("alpha", "alpha"); w.core.set_crew_approvers(approvers(&lead, &[])); - signed_in(&w).await; + // The channel stays open for the whole test, so the service has a shell to answer to. + let (lines, answers) = mpsc::unbounded_channel(); + w.core.shell.attach(lines); + std::mem::forget(answers); (w, id) } @@ -99,7 +115,7 @@ async fn a_denied_command_is_held_once_and_a_retry_does_not_add_another() { async fn approving_once_lets_exactly_one_retry_of_the_same_command_through() { let (w, id) = held_world().await; let hold = hold_of(&w, &id, "make deploy").await; - let nonce = nonce_for(&w, &hold).await; + let nonce = grant_for(&w, &hold); let approved = w .core @@ -120,7 +136,7 @@ async fn approving_once_lets_exactly_one_retry_of_the_same_command_through() { } let approved_row = &events(&w, &id, "approved_once")[0]; assert_eq!(approved_row.body["decided_by"], "person"); - assert_eq!(approved_row.body["via"], "local_session"); + assert_eq!(approved_row.body["via"], "desktop_presence"); let allowed = events(&w, &id, "allowed"); assert_eq!(allowed[0].body["decided_by"], "person"); assert_eq!(allowed[0].body["hold"], hold); @@ -132,7 +148,7 @@ async fn an_approval_covers_only_the_exact_command_arguments_and_thread() { for case in ["command", "spacing", "arguments", "paths", "thread"] { let (w, id) = held_world().await; let hold = hold_of(&w, &id, "make deploy").await; - let nonce = nonce_for(&w, &hold).await; + let nonce = grant_for(&w, &hold); w.core .autopilot_approve_once(&hold, Some(&nonce)) .await @@ -159,64 +175,213 @@ async fn an_approval_covers_only_the_exact_command_arguments_and_thread() { } #[tokio::test] -async fn an_approval_needs_this_holds_own_unused_confirmation() { +async fn an_approval_needs_this_holds_own_unused_grant() { let (w, id) = held_world().await; let hold = hold_of(&w, &id, "make deploy").await; let other = hold_of(&w, &id, "make ship").await; - // No nonce at all: a caller with only the service token. + // No nonce at all: a caller with only the service token. A wrong one, and one that nothing granted. let none = w.core.autopilot_approve_once(&hold, None).await; - assert!(matches!(none, Err(CoreError::ReauthRequired))); + assert!(matches!(none, Err(CoreError::PresenceRequired))); let wrong = w.core.autopilot_approve_once(&hold, Some("0000")).await; - assert!(matches!(wrong, Err(CoreError::ReauthRequired))); + assert!(matches!(wrong, Err(CoreError::PresenceRequired))); + let unknown = w + .core + .autopilot_approve_once("hold-nope", Some(NONCE)) + .await; + assert!(matches!(unknown, Err(CoreError::PresenceRequired))); - // A confirmation made for another hold does not cover this one. - let for_other = nonce_for(&w, &other).await; + // A grant made for another hold does not cover this one. + let for_other = grant_for(&w, &other); let swapped = w.core.autopilot_approve_once(&hold, Some(&for_other)).await; - assert!(matches!(swapped, Err(CoreError::ReauthRequired))); - - // A confirmation for something else in the account does not either. - let device = w - .core - .account - .confirm( - ConfirmFor::RemoveDevice(hold.clone()), - Password::new(account_password()), - ) - .await - .unwrap(); - let wrong_action = w.core.autopilot_approve_once(&hold, Some(&device)).await; - assert!(matches!(wrong_action, Err(CoreError::ReauthRequired))); + assert!(matches!(swapped, Err(CoreError::PresenceRequired))); assert_eq!(state_of(&w, &hold), State::Held, "nothing was approved"); - // Used up by its first use: a replay with the same nonce fails. - let nonce = nonce_for(&w, &hold).await; + // A wrong nonce does not burn the grant: the right one still works afterwards. + let nonce = grant_for(&w, &hold); + let wrong = w + .core + .autopilot_approve_once(&hold, Some("fedcba9876543210fedcba9876543210")) + .await; + assert!(matches!(wrong, Err(CoreError::PresenceRequired))); w.core .autopilot_approve_once(&hold, Some(&nonce)) .await .unwrap(); + + // Used up by its first use: a replay with the same nonce fails, and so does a fresh grant on a decided hold. let replay = w.core.autopilot_approve_once(&hold, Some(&nonce)).await; - assert!(matches!(replay, Err(CoreError::ReauthRequired))); - // And a fresh confirmation cannot approve what is already decided. - let fresh = nonce_for(&w, &hold).await; - let twice = w.core.autopilot_approve_once(&hold, Some(&fresh)).await; - assert!(matches!(twice, Err(CoreError::InvalidTransition { .. }))); - - // Signed out, nothing can be approved. - let third = nonce_for(&w, &other).await; - w.core.account.forget_confirmation(); - let gone = w.core.autopilot_approve_once(&other, Some(&third)).await; - assert!(matches!(gone, Err(CoreError::ReauthRequired))); + assert!(matches!(replay, Err(CoreError::PresenceRequired))); + let refused = describe(&w, &hold, unix_now()); + assert_eq!(refused["type"], "presence_refused"); + assert_eq!(refused["code"], "gone"); + let twice = w.core.autopilot_approve_once(&hold, Some(NONCE)).await; + assert!(matches!(twice, Err(CoreError::PresenceRequired))); +} + +#[tokio::test] +async fn the_shell_is_told_what_the_dialog_shows_and_nothing_from_the_window() { + let (w, id) = held_world().await; + let command = format!("make deploy -- echo {KEY_MARKER}"); + request(&w, &id, ActionKind::Command, &command).await; + let hold = w.core.autopilot_held().unwrap().items.remove(0); + let shown = describe(&w, &hold.id, unix_now()); + assert_eq!(shown["type"], "presence_hold"); + assert_eq!(shown["request_id"], "r-1"); + assert_eq!(shown["hold_id"], hold.id); + assert_eq!(shown["task_id"], id); + assert_eq!(shown["thread_id"], w.worker(&id)); + assert_eq!(shown["command_shown"], hold.command); + assert!( + !shown["command_shown"] + .as_str() + .unwrap() + .contains(KEY_MARKER) + ); + assert_eq!(shown["command_sha256"].as_str().unwrap().len(), 64); + assert_eq!(shown["task_title"], "Add a parse_duration function."); + let run = w.core.store.crew_run(&id).unwrap().unwrap(); + assert_eq!(shown["worker_name"], run.member); + assert!(!shown.to_string().contains(KEY_MARKER)); + // A line that is not for this side is left for the next reader, and a request id the shell cannot have made is dropped. + assert!( + !w.core + .presence_line_at(r#"{"type":"open_link"}"#, unix_now()) + ); + assert!(!w.core.presence_line_at("not json", unix_now())); + assert!( + w.core.presence_line_at( + &json!({ "type": "presence_describe", "request_id": "bad id!", "hold_id": hold.id }) + .to_string(), + unix_now() + ) + ); +} + +#[tokio::test] +async fn a_grant_is_recorded_only_for_the_hold_as_it_stands_now() { + let (w, id) = held_world().await; + let hold = hold_of(&w, &id, "make deploy").await; + let shown = describe(&w, &hold, unix_now()); + let grant = |what: Value, now: i64| { + let mut line = json!({ + "type": "presence_grant", "request_id": "r-2", "nonce": NONCE, "hold_id": hold, + "thread_id": shown["thread_id"], "task_id": shown["task_id"], + "command_sha256": shown["command_sha256"], + }); + line.as_object_mut() + .unwrap() + .extend(what.as_object().unwrap().clone()); + shell_says(&w, &line, now) + }; + let code = |answer: Value| { + assert_eq!(answer["type"], "presence_refused", "{answer}"); + answer["code"].as_str().unwrap().to_owned() + }; + // Another command's hash, another thread, another task, another hold: nothing is recorded. + assert_eq!( + code(grant( + json!({ "command_sha256": "0".repeat(64) }), + unix_now() + )), + "mismatch" + ); + assert_eq!( + code(grant(json!({ "thread_id": "another-thread" }), unix_now())), + "mismatch" + ); + assert_eq!( + code(grant(json!({ "task_id": "another-task" }), unix_now())), + "mismatch" + ); + assert_eq!( + code(grant(json!({ "hold_id": "hold-nope" }), unix_now())), + "not_found" + ); + assert_eq!( + code(grant(json!({ "hold_id": "../x" }), unix_now())), + "invalid" + ); + assert_eq!( + code(grant(json!({ "nonce": "short" }), unix_now())), + "invalid" + ); + assert_eq!( + code(grant(json!({ "nonce": NONCE.to_uppercase() }), unix_now())), + "invalid" + ); + assert!(matches!( + w.core.autopilot_approve_once(&hold, Some(NONCE)).await, + Err(CoreError::PresenceRequired) + )); + assert_eq!(w.core.presence.len(), 0); + + // The hold ends before the person clicks: the grant is refused, and the end of the task is the same. + w.core.autopilot_dismiss_held(&hold).unwrap(); + assert_eq!(code(grant(json!({}), unix_now())), "gone"); + assert_eq!(w.core.presence.len(), 0); + let hold2 = hold_of(&w, &id, "make ship").await; + let task = w.core.store.crew_task(&id).unwrap().unwrap(); + w.core.autopilot_stop( + &task, + StopReason::TooManyDenials, + "too_many_denials", + "ended", + ); + assert_eq!(describe(&w, &hold2, unix_now())["code"], "gone"); } #[tokio::test] -async fn nothing_can_be_approved_without_an_account() { +async fn a_second_grant_replaces_the_first_and_a_grant_runs_out() { + let (w, id) = held_world().await; + let hold = hold_of(&w, &id, "make deploy").await; + let first = grant_for(&w, &hold); + let second = grant_at(&w, &hold, "fedcba9876543210fedcba9876543210", unix_now()); + assert_eq!(w.core.presence.len(), 1); + let old = w.core.autopilot_approve_once(&hold, Some(&first)).await; + assert!( + matches!(old, Err(CoreError::PresenceRequired)), + "the older grant is gone" + ); + + // Sixty seconds on, the grant has run out; the hold itself is still open. + let late = unix_now() + PRESENCE_LIFETIME_SECS; + let expired = w + .core + .autopilot_approve_once_at(&hold, Some(&second), late) + .await; + assert!(matches!(expired, Err(CoreError::PresenceRequired))); + assert_eq!(state_of(&w, &hold), State::Held); + let live = w + .core + .autopilot_approve_once_at( + &hold, + Some(&second), + unix_now() + PRESENCE_LIFETIME_SECS - 2, + ) + .await + .unwrap(); + assert_eq!(live.state, State::Approved); +} + +#[tokio::test] +async fn without_a_desktop_shell_nothing_can_be_approved() { let (w, id) = running().await; let lead = Scripted::no("alpha", "alpha"); w.core.set_crew_approvers(approvers(&lead, &[])); let hold = hold_of(&w, &id, "make deploy").await; - let refused = w.core.autopilot_approve_once(&hold, Some("nonce")).await; - assert!(matches!(refused, Err(CoreError::ReauthRequired))); + for nonce in [None, Some(NONCE)] { + let refused = w.core.autopilot_approve_once(&hold, nonce).await; + assert!( + matches!(refused, Err(CoreError::DesktopRequired)), + "{nonce:?}" + ); + } + assert_eq!(state_of(&w, &hold), State::Held); + assert_eq!(CoreError::DesktopRequired.code(), "desktop_required"); + assert_eq!(CoreError::PresenceRequired.code(), "presence_required"); + assert_eq!(CoreError::DesktopRequired.status(), 403); + assert_eq!(CoreError::PresenceRequired.status(), 403); } #[tokio::test] @@ -224,6 +389,13 @@ async fn a_hold_and_an_approval_expire() { let (w, id) = held_world().await; let hold = hold_of(&w, &id, "make deploy").await; let late = unix_now() + HELD_LIFETIME_SECS + 1; + // The grant was made just before the hold ran out, so it is still live when the hold is over: the answer is `gone`. + let nonce = grant_at(&w, &hold, NONCE, late - 2); + let gone = w + .core + .autopilot_approve_once_at(&hold, Some(&nonce), late) + .await; + assert!(matches!(gone, Err(CoreError::Gone(_)))); assert_eq!( w.core .autopilot_held_at(late) @@ -235,18 +407,17 @@ async fn a_hold_and_an_approval_expire() { .state, State::Expired ); - let nonce = nonce_for(&w, &hold).await; - let gone = w - .core - .autopilot_approve_once_at(&hold, Some(&nonce), late) - .await; - assert!(matches!(gone, Err(CoreError::Gone(_)))); + assert_eq!( + w.core.presence.len(), + 1, + "a refusal does not use the grant up" + ); assert_eq!(events(&w, &id, "hold_ended").len(), 1); // An approval whose window has passed lets nothing through: it was made, by the clock, long ago. let hold = hold_of(&w, &id, "make ship").await; - let nonce = nonce_for(&w, &hold).await; let past = unix_now() - APPROVAL_LIFETIME_SECS - 5; + let nonce = grant_at(&w, &hold, NONCE, past); w.core .autopilot_approve_once_at(&hold, Some(&nonce), past) .await @@ -259,7 +430,7 @@ async fn a_hold_and_an_approval_expire() { async fn a_held_command_of_an_ended_task_is_gone() { let (w, id) = held_world().await; let hold = hold_of(&w, &id, "make deploy").await; - let nonce = nonce_for(&w, &hold).await; + let nonce = grant_for(&w, &hold); let task = w.core.store.crew_task(&id).unwrap().unwrap(); w.core.autopilot_stop( &task, @@ -289,7 +460,7 @@ async fn the_always_denied_list_is_checked_again_at_approval_time() { let hold = hold_of(&w, &id, "git push").await; // The checkout moves to main before the person answers: the same push would now be a push to main (N5). run_git(&worktree, &["checkout", "-q", "-B", "main"]); - let nonce = nonce_for(&w, &hold).await; + let nonce = grant_for(&w, &hold); let refused = w.core.autopilot_approve_once(&hold, Some(&nonce)).await; match refused { Err(CoreError::PolicyDenied(words)) => assert!(words.contains("N5"), "{words}"), @@ -357,16 +528,18 @@ async fn no_agent_can_approve_not_the_lead_nor_a_request_to_the_route() { ); } - // A yes from the lead does not make an approval either: only the person's call does, and it needs the confirmation. + // A yes from the lead does not make an approval either: only the person's call does, and it needs the desktop app's grant. let (w, id) = running().await; let lead = Scripted::no("alpha", "alpha"); w.core.set_crew_approvers(approvers(&lead, &[])); - signed_in(&w).await; + let (lines, answers) = mpsc::unbounded_channel(); + w.core.shell.attach(lines); + std::mem::forget(answers); let hold = hold_of(&w, &id, "make deploy").await; let lead = Scripted::yes("alpha", "alpha"); w.core.set_crew_approvers(approvers(&lead, &[])); let refused = w.core.autopilot_approve_once(&hold, None).await; - assert!(matches!(refused, Err(CoreError::ReauthRequired))); + assert!(matches!(refused, Err(CoreError::PresenceRequired))); assert_eq!(state_of(&w, &hold), State::Held); } @@ -395,11 +568,35 @@ async fn deny_needs_no_password_and_the_same_command_is_not_held_again() { let verdict = request(&w, &id, ActionKind::Command, "make deploy").await; assert!(matches!(verdict, Verdict::Deny(_))); assert_eq!(w.core.autopilot_held().unwrap().items.len(), 1); - let nonce = nonce_for(&w, &hold).await; + // A person who clicked Approve before the deny lost the race: the hold is decided and the grant is not used. + let (w, id) = held_world().await; + let hold = hold_of(&w, &id, "make ship").await; + let nonce = grant_for(&w, &hold); + w.core.autopilot_deny_held(&hold).unwrap(); assert!(matches!( w.core.autopilot_approve_once(&hold, Some(&nonce)).await, Err(CoreError::InvalidTransition { .. }) )); + assert_eq!(w.core.presence.len(), 1); + assert_eq!(state_of(&w, &hold), State::Denied); +} + +#[tokio::test] +async fn a_deny_after_an_approval_loses_and_is_one_row() { + let (w, id) = held_world().await; + let hold = hold_of(&w, &id, "make deploy").await; + let nonce = grant_for(&w, &hold); + w.core + .autopilot_approve_once(&hold, Some(&nonce)) + .await + .unwrap(); + assert!(matches!( + w.core.autopilot_deny_held(&hold), + Err(CoreError::InvalidTransition { .. }) + )); + assert_eq!(state_of(&w, &hold), State::Approved); + assert_eq!(events(&w, &id, "denied_by_person").len(), 0); + assert_eq!(events(&w, &id, "approved_once").len(), 1); } #[tokio::test] @@ -426,7 +623,7 @@ async fn a_racing_approve_and_deny_have_exactly_one_winner() { for round in 0..8 { let (w, id) = held_world().await; let hold = hold_of(&w, &id, &format!("make step-{round}")).await; - let nonce = nonce_for(&w, &hold).await; + let nonce = grant_for(&w, &hold); let (approve, deny) = tokio::join!(w.core.autopilot_approve_once(&hold, Some(&nonce)), async { w.core.autopilot_deny_held(&hold) @@ -482,7 +679,7 @@ async fn the_answer_keys_text_is_withheld_from_the_command_the_person_reads() { !item.command.contains(KEY_MARKER), "the shown command held the key's text" ); - let nonce = nonce_for(&w, &item.id).await; + let nonce = grant_for(&w, &item.id); w.core .autopilot_approve_once(&item.id, Some(&nonce)) .await @@ -509,9 +706,11 @@ async fn a_command_too_long_to_read_is_held_but_cannot_be_approved() { assert!(matches!(verdict, Verdict::Deny(_))); let item = w.core.autopilot_held().unwrap().items.remove(0); assert!(!item.readable); - let nonce = nonce_for(&w, &item.id).await; - let refused = w.core.autopilot_approve_once(&item.id, Some(&nonce)).await; - assert!(matches!(refused, Err(CoreError::Invalid(_)))); + // The dialog is never offered it, and no grant can be made for it. + let refused = describe(&w, &item.id, unix_now()); + assert_eq!(refused["code"], "unreadable"); + let refused = w.core.autopilot_approve_once(&item.id, Some(NONCE)).await; + assert!(matches!(refused, Err(CoreError::PresenceRequired))); assert_eq!(state_of(&w, &item.id), State::Held); } @@ -539,26 +738,3 @@ async fn with_the_flag_off_none_of_it_exists() { Err(CoreError::NotFound(_)) )); } - -#[tokio::test] -async fn approve_once_is_refused_while_a_seeded_stand_in_account_is_signed_in() { - let (w, id) = running().await; - let lead = Scripted::no("alpha", "alpha"); - w.core.set_crew_approvers(approvers(&lead, &[])); - // The seeded account's password is public, so anything holding the token could confirm as it. - let outcome = w - .core - .account - .sign_in("dev@example.com", Password::new(account_password())) - .await - .unwrap(); - assert_eq!(outcome, orbyn_contracts::AccountSignInOutcome::Signed); - let hold = hold_of(&w, &id, "make deploy").await; - let nonce = nonce_for(&w, &hold).await; - let refused = w.core.autopilot_approve_once(&hold, Some(&nonce)).await; - match refused { - Err(CoreError::PolicyDenied(words)) => assert!(words.contains("stand-in"), "{words}"), - other => panic!("{other:?}"), - } - assert_eq!(state_of(&w, &hold), State::Held); -} diff --git a/crates/orbyn-core/src/crew/tests/harness.rs b/crates/orbyn-core/src/crew/tests/harness.rs index 4993047e..207187be 100644 --- a/crates/orbyn-core/src/crew/tests/harness.rs +++ b/crates/orbyn-core/src/crew/tests/harness.rs @@ -250,9 +250,6 @@ async fn world_full( config.engines.known_engine_ids = Some(Vec::new()); config.crew_preview = preview; config.crew_confine_workers = confine; - // The development stand-in for the Engine, so a test can sign in and "Confirm it's you" (approve once needs it). - config.account_gate = crate::account::AccountGate::LocalOnly; - config.account_backend = crate::account::AccountBackendKind::Development; if tiered { let words = crate::crew::TierWords { lead: Some("lead".into()), diff --git a/crates/orbyn-core/src/error.rs b/crates/orbyn-core/src/error.rs index 87d7d7b5..e1141960 100644 --- a/crates/orbyn-core/src/error.rs +++ b/crates/orbyn-core/src/error.rs @@ -122,6 +122,13 @@ pub enum CoreError { message: String, detail: serde_json::Value, }, + /// An action that only a person at the desktop app may give: the service has no desktop shell (it was started without one, or + /// as a remote service), so nothing can be approved. + #[error("Approving a command needs the Orbyn desktop app on this computer.")] + DesktopRequired, + /// The desktop app has not recorded a person's approval for this hold, or it is used, expired or for something else. + #[error("Approve this command in the Orbyn desktop app first.")] + PresenceRequired, /// The thing existed and is over: a held command that expired or whose task ended. #[error("{0}")] Gone(String), @@ -168,6 +175,8 @@ impl CoreError { Self::PackTampered(_) => "pack_tampered", Self::AccountDisabled => "account_disabled", Self::ReauthRequired => "reauth_required", + Self::DesktopRequired => "desktop_required", + Self::PresenceRequired => "presence_required", Self::Gone(_) => "gone", Self::Account { code, .. } | Self::PatternRefused { code, .. } => code, Self::ProjectExists(_) => "project_exists", @@ -188,7 +197,9 @@ impl CoreError { | Self::PolicyDenied(_) | Self::WorkspaceRefused(_) | Self::AccountDisabled - | Self::ReauthRequired => 403, + | Self::ReauthRequired + | Self::DesktopRequired + | Self::PresenceRequired => 403, Self::Account { code, .. } => match *code { "account_offline" => 503, "rate_limited" => 429, @@ -292,6 +303,8 @@ impl CoreError { Self::DataDirLocked => Self::DataDirLocked, Self::AccountDisabled => Self::AccountDisabled, Self::ReauthRequired => Self::ReauthRequired, + Self::DesktopRequired => Self::DesktopRequired, + Self::PresenceRequired => Self::PresenceRequired, Self::Account { code, message, diff --git a/crates/orbyn-core/src/host/README.md b/crates/orbyn-core/src/host/README.md index 6f7c8d57..7feba19d 100644 --- a/crates/orbyn-core/src/host/README.md +++ b/crates/orbyn-core/src/host/README.md @@ -7,6 +7,7 @@ The operating system side: processes, their environment, terminals and the deskt | `process` | Child processes as owned resources that die with their owner | | `child_env` | What a spawned program inherits | | `confine` | The restriction a crew worker's engine process runs under (macOS), and the refusal elsewhere | +| `presence` | One in-memory record per action of a person's approval given in the desktop app: bound fields, one nonce, 60 seconds, used once | | `terminal` | PTY sessions with bounded scrollback; ConPTY on Windows | | `shell` | The desktop shell's side of the service | diff --git a/crates/orbyn-core/src/host/mod.rs b/crates/orbyn-core/src/host/mod.rs index 97e185bb..d12b1f6d 100644 --- a/crates/orbyn-core/src/host/mod.rs +++ b/crates/orbyn-core/src/host/mod.rs @@ -2,6 +2,7 @@ pub mod child_env; pub mod confine; +pub mod presence; pub mod process; pub mod shell; pub mod terminal; diff --git a/crates/orbyn-core/src/host/presence.rs b/crates/orbyn-core/src/host/presence.rs new file mode 100644 index 00000000..6a45c591 --- /dev/null +++ b/crates/orbyn-core/src/host/presence.rs @@ -0,0 +1,208 @@ +//! A person's approval, given in the desktop app, kept in memory until the action it was for asks for it. +//! +//! The desktop shell shows a native dialog, and when the person clicks Approve it mints a one-time nonce and sends it to the +//! service on the channel only the shell holds (the service's stdin, see `shell`). The service records one grant per action here; +//! the page then sends the same nonce with the action's own request, and the action takes the grant, once. A caller that only +//! speaks HTTP to the service, with or without the service token, has no way to put a grant here. +//! +//! **Not OS-level presence.** What this proves is that the grant came in on the shell's channel and carries a nonce the page was +//! handed after it. A process of the same OS user can still drive the desktop app's native dialog, read the shell's memory or +//! write into its pipe (docs/crew.md, "Approving once: local presence"). +//! +//! A grant is for exactly the fields it was made with, lives [`PRESENCE_LIFETIME_SECS`], is replaced by a later grant for the same +//! action, is used up by its first use and is never written to disk, so a restart drops every grant. + +use std::sync::{Mutex, MutexGuard, PoisonError}; + +use crate::util::same_secret; + +/// How long a person's approval waits for the page to use it, in seconds. +pub const PRESENCE_LIFETIME_SECS: i64 = 60; +/// Grants kept at once; the oldest go first. +const KEPT: usize = 64; + +/// What a person approved. Every field must match when the grant is used. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum PresenceFor { + /// One held command, once: the hold, the worker's thread, the task and the hold's binding hash. + CrewApproveOnce { + hold: String, + thread: String, + task: String, + command_sha256: String, + }, +} + +impl PresenceFor { + /// The action a grant belongs to: a later grant for the same key replaces the earlier one. + fn key(&self) -> &str { + match self { + Self::CrewApproveOnce { hold, .. } => hold, + } + } +} + +struct Grant { + nonce: String, + what: PresenceFor, + expires: i64, +} + +#[derive(Default)] +pub struct PresenceBook { + grants: Mutex>, +} + +fn lock(grants: &Mutex>) -> MutexGuard<'_, Vec> { + grants.lock().unwrap_or_else(PoisonError::into_inner) +} + +/// Whether `text` is a nonce the shell may mint: 16 random bytes as lowercase hex. +pub fn is_nonce(text: &str) -> bool { + text.len() == 32 + && text + .bytes() + .all(|byte| byte.is_ascii_digit() || (b'a'..=b'f').contains(&byte)) +} + +impl PresenceBook { + /// Records a grant, replacing the earlier one for the same action. + pub fn put(&self, what: PresenceFor, nonce: String, now: i64) { + let mut grants = lock(&self.grants); + grants.retain(|g| g.expires > now && g.what.key() != what.key()); + grants.push(Grant { + nonce, + what, + expires: now + PRESENCE_LIFETIME_SECS, + }); + while grants.len() > KEPT { + grants.remove(0); + } + } + + /// Whether a live grant for exactly this action carries this nonce. It does not use the grant up, so a refused request + /// does not burn it. + pub fn confirmed_for(&self, what: &PresenceFor, nonce: Option<&str>, now: i64) -> bool { + let grants = lock(&self.grants); + Self::matching(&grants, what, nonce, now).is_some() + } + + /// Uses the grant up if it is live, for exactly this action, and carries this nonce. + pub fn take(&self, what: &PresenceFor, nonce: Option<&str>, now: i64) -> bool { + let mut grants = lock(&self.grants); + let Some(at) = Self::matching(&grants, what, nonce, now) else { + return false; + }; + grants.remove(at); + true + } + + fn matching( + grants: &[Grant], + what: &PresenceFor, + nonce: Option<&str>, + now: i64, + ) -> Option { + let sent = nonce?; + grants + .iter() + .position(|g| g.expires > now && g.what == *what && same_secret(&g.nonce, sent)) + } + + /// Grants held now (for tests). + #[cfg(test)] + pub(crate) fn len(&self) -> usize { + lock(&self.grants).len() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const NONCE: &str = "0123456789abcdef0123456789abcdef"; + + fn what(hold: &str) -> PresenceFor { + PresenceFor::CrewApproveOnce { + hold: hold.into(), + thread: "w-1".into(), + task: "t-1".into(), + command_sha256: "abc".into(), + } + } + + fn with(change: impl FnOnce(&mut String, &mut String, &mut String)) -> PresenceFor { + let mut other = what("h-1"); + let PresenceFor::CrewApproveOnce { + thread, + task, + command_sha256, + .. + } = &mut other; + change(thread, task, command_sha256); + other + } + + #[test] + fn a_grant_is_used_once_and_only_with_its_nonce() { + let book = PresenceBook::default(); + book.put(what("h-1"), NONCE.into(), 100); + assert!(!book.confirmed_for(&what("h-1"), None, 101)); + assert!(!book.confirmed_for(&what("h-1"), Some("0123456789abcdef0123456789abcdee"), 101)); + assert!(!book.take(&what("h-1"), Some("short"), 101)); + assert_eq!(book.len(), 1, "a wrong nonce does not burn the grant"); + assert!(book.confirmed_for(&what("h-1"), Some(NONCE), 101)); + assert!( + book.confirmed_for(&what("h-1"), Some(NONCE), 102), + "asking does not use it" + ); + assert!(book.take(&what("h-1"), Some(NONCE), 102)); + assert!(!book.take(&what("h-1"), Some(NONCE), 103), "used up"); + } + + #[test] + fn a_grant_matches_only_the_fields_it_was_made_with() { + let book = PresenceBook::default(); + book.put(what("h-1"), NONCE.into(), 100); + for other in [ + with(|thread, _, _| *thread = "w-2".into()), + with(|_, task, _| *task = "t-2".into()), + with(|_, _, hash| *hash = "changed".into()), + what("h-2"), + ] { + assert!(!book.confirmed_for(&other, Some(NONCE), 101), "{other:?}"); + } + assert!(book.confirmed_for(&what("h-1"), Some(NONCE), 101)); + } + + #[test] + fn a_grant_expires_and_a_later_grant_replaces_the_earlier() { + let book = PresenceBook::default(); + book.put(what("h-1"), NONCE.into(), 100); + let live = 100 + PRESENCE_LIFETIME_SECS - 1; + assert!(book.confirmed_for(&what("h-1"), Some(NONCE), live)); + assert!(!book.confirmed_for(&what("h-1"), Some(NONCE), live + 1)); + let second = "fedcba9876543210fedcba9876543210"; + book.put(what("h-1"), NONCE.into(), 200); + book.put(what("h-1"), second.into(), 201); + assert_eq!(book.len(), 1); + assert!( + !book.confirmed_for(&what("h-1"), Some(NONCE), 202), + "the older one is gone" + ); + assert!(book.confirmed_for(&what("h-1"), Some(second), 202)); + } + + #[test] + fn grants_are_bounded_and_nonces_have_one_shape() { + let book = PresenceBook::default(); + for n in 0..KEPT + 5 { + book.put(what(&format!("h-{n}")), NONCE.into(), 100); + } + assert_eq!(book.len(), KEPT); + assert!(is_nonce(NONCE)); + assert!(!is_nonce("0123456789ABCDEF0123456789ABCDEF")); + assert!(!is_nonce("0123456789abcdef")); + assert!(!is_nonce(&format!("{NONCE}0"))); + } +} diff --git a/crates/orbyn-core/src/host/shell.rs b/crates/orbyn-core/src/host/shell.rs index 23acb252..5a92329e 100644 --- a/crates/orbyn-core/src/host/shell.rs +++ b/crates/orbyn-core/src/host/shell.rs @@ -89,6 +89,11 @@ impl ShellBridge { *lock(&self.lines) = Some(lines); } + /// Whether a desktop shell is connected: the service was started with the parent channel. + pub fn attached(&self) -> bool { + lock(&self.lines).is_some() + } + /// Sends one line. False means no shell is listening. pub fn send(&self, line: Value) -> bool { lock(&self.lines) @@ -392,7 +397,7 @@ impl Core { } } -fn is_id(text: &str) -> bool { +pub(crate) fn is_id(text: &str) -> bool { !text.is_empty() && text.len() <= ID_LIMIT && text diff --git a/crates/orbyn-core/src/util.rs b/crates/orbyn-core/src/util.rs index f3902831..49ea4318 100644 --- a/crates/orbyn-core/src/util.rs +++ b/crates/orbyn-core/src/util.rs @@ -97,6 +97,16 @@ pub fn secrets_match(presented: &str, expected_sha256_hex: &str) -> bool { == 0 } +/// Compares two secrets of the same text form without stopping at the first difference. +pub(crate) fn same_secret(left: &str, right: &str) -> bool { + left.len() == right.len() + && left + .bytes() + .zip(right.bytes()) + .fold(0u8, |diff, (a, b)| diff | (a ^ b)) + == 0 +} + /// Writes a file readable only by the current user. /// /// On Unix the mode is set at creation. Windows has no mode bits: the file diff --git a/crates/orbynd/src/http/access.rs b/crates/orbynd/src/http/access.rs index d3a123c2..9c82953b 100644 --- a/crates/orbynd/src/http/access.rs +++ b/crates/orbynd/src/http/access.rs @@ -216,8 +216,8 @@ pub async fn guard_origin( } /// The headers a desktop page may send across origins. `x-orbyn-confirm` carries the nonce a -/// confirmed account action sends back; allowing the header grants nothing alone, the nonce is -/// still checked by the account guard. +/// confirmed account action, or a held command the person approved in the desktop app, sends back; allowing the header grants +/// nothing alone, the nonce is still checked by the account guard or the core's presence grant. const NATIVE_HEADERS: &str = "authorization, content-type, accept, last-event-id, range, if-range, if-none-match, x-orbyn-confirm"; fn native_preflight(request: &Request) -> bool { diff --git a/crates/orbynd/src/main.rs b/crates/orbynd/src/main.rs index 97110296..c54156da 100644 --- a/crates/orbynd/src/main.rs +++ b/crates/orbynd/src/main.rs @@ -180,7 +180,7 @@ fn read_parent(core: Arc, closed: oneshot::Sender<()>) { for line in std::io::stdin().lock().lines() { match line { Ok(line) => { - if !core.shell.on_line(&line) { + if !core.shell.on_line(&line) && !core.presence_line(&line) { core.updates.on_shell_line(&line); } } diff --git a/crates/orbynd/src/routes/crew.rs b/crates/orbynd/src/routes/crew.rs index 1c9fa709..0cddc2e9 100644 --- a/crates/orbynd/src/routes/crew.rs +++ b/crates/orbynd/src/routes/crew.rs @@ -20,15 +20,14 @@ //! //! **No agent approves; a person may approve once.** A command the chain denied is held (`GET /api/crew/approvals/held`). The person //! may approve that exact command once (`POST /api/crew/approvals/{id}/approve-once`), deny it or dismiss it. Approving needs the -//! account's "Confirm it's you" for this hold: the window asks `POST /api/account/confirm` (`approve_device`, `device_id` -//! `crew-approve-once:`) and sends the nonce back in `x-orbyn-confirm`, so a caller that holds only the service token -//! (#441) cannot approve. It protects against that caller and nothing more: a worker that can drive the window or read the -//! password defeats it. **In release builds approve-once always fails with `account_disabled` (the account is off); in dev builds -//! the stand-in account's password is public, so the protection is only as strong as that password, and approve-once is refused -//! while a seeded stand-in account is signed in.** Deny and dismiss need no password (they only refuse), so a token holder can -//! deny or dismiss a hold, never approve one; their ledger rows say `local_caller`, not `person`. Remote viewers are refused for -//! all three. Until #441 (workers can read the service token and the data folder) is fixed, the Preview flag stays for -//! development only. +//! person's click in the desktop app's own native dialog: the shell records a one-time grant with the service on the channel only +//! the shell holds, and the page sends the grant's nonce in `x-orbyn-confirm`. A service without a desktop shell, and a remote one, +//! answers `403 desktop_required`; a missing, wrong, used, expired or mismatched nonce is `403 presence_required` and does not use +//! the grant up. No caller that only speaks HTTP to the service can approve once, with or without the service token. This is not +//! OS-level presence (docs/crew.md, "Approving once: local presence"). Deny and dismiss need no grant (they only refuse), so a +//! token holder can deny or dismiss a hold, never approve one; their ledger rows say `local_caller`, not `person`. Remote viewers +//! are refused for all three. Until #441 (workers can read the service token and the data folder) is fixed, the Preview flag stays +//! for development only. use axum::Json; use axum::Router; @@ -76,7 +75,7 @@ pub const GUARDS: &[(&str, &str, &str)] = &[ ( "POST", "/api/crew/approvals/{id}/approve-once", - "core autopilot_approve_once: the account's confirmation for this hold (x-orbyn-confirm), used up; the hold open, its task running, the command read whole, the always-denied list again, one decision; local only", + "core autopilot_approve_once: a desktop shell, the person's grant for this hold as it stands (x-orbyn-confirm), used up; the hold open, its task running, the command read whole, the always-denied list again, one decision; local only", ), ( "POST", @@ -187,6 +186,12 @@ async fn local_only( request: Request, next: Next, ) -> Result { + if state.remote && request.uri().path().ends_with("/approve-once") { + return Err(ApiFailure::forbidden( + "desktop_required", + "Approving a command needs the Orbyn desktop app on this computer.", + )); + } if state.remote { return Err(ApiFailure::forbidden( "local_only", @@ -340,7 +345,7 @@ async fn held(State(state): State) -> ApiResult { Ok(Json(state.core.autopilot_held()?)) } -/// The header the confirmation's nonce comes back in: what the account's "Confirm it's you" answered with. +/// The header the person's grant nonce comes in: what the desktop app handed the page after the person clicked Approve. const CONFIRM_HEADER: &str = "x-orbyn-confirm"; async fn approve_once( diff --git a/crates/orbynd/src/routes/crew_held_tests.rs b/crates/orbynd/src/routes/crew_held_tests.rs index 4440db65..2097d576 100644 --- a/crates/orbynd/src/routes/crew_held_tests.rs +++ b/crates/orbynd/src/routes/crew_held_tests.rs @@ -1,18 +1,19 @@ -//! A person's one-time answer to a held command over HTTP: the confirmation, the single use, the remote refusal, and the +//! A person's one-time answer to a held command over HTTP: the desktop app's grant, the single use, the remote refusal, and the //! refusals that make an agent's call useless. The table test in `crew_tests.rs` already walks every route with the flag off -//! (404 for every method) and checks the marker and the guards; this file drives the three answers themselves. The account is -//! the development stand-in for the Engine, and the password is assembled at run time. +//! (404 for every method) and checks the marker and the guards; this file drives the three answers themselves. The desktop +//! shell is played in this process: a channel stands in for the service's stdout and the shell's lines go to +//! `Core::presence_line`, as `main` hands them from stdin. `tests/presence_stdio.rs` runs the real binary. use std::path::PathBuf; use std::sync::Arc; use orbyn_contracts as wire; -use orbyn_core::account::{AccountBackendKind, AccountGate, Password}; use orbyn_core::crew::Stamper; use orbyn_core::crew::fixtures::hold_command; use orbyn_core::{Core, CoreConfig}; use serde_json::{Value, json}; use tokio::net::TcpListener; +use tokio::sync::mpsc; use super::router; use crate::http::AppState; @@ -22,18 +23,28 @@ struct Served { port: u16, core: Arc, dir: PathBuf, + /// What the service says to the shell. + shell: Option>>, } +/// A service with a desktop shell attached, as `--parent-stdin` makes one. async fn serve(name: &str, remote: bool) -> Served { + serve_with(name, remote, true).await +} + +async fn serve_with(name: &str, remote: bool, desktop: bool) -> Served { let dir = std::env::temp_dir().join(format!("orbynd-held-{name}-{}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); let mut config = CoreConfig::new(dir.clone(), "http://127.0.0.1:1".into()); config.crew_preview = true; config.crew_confine_workers = false; - config.account_gate = AccountGate::LocalOnly; - config.account_backend = AccountBackendKind::Development; let core = Arc::new(Core::open(config).unwrap()); + let shell = desktop.then(|| { + let (lines, answers) = mpsc::unbounded_channel(); + core.shell.attach(lines); + std::sync::Mutex::new(answers) + }); let app = router(AppState { core: core.clone(), access: Arc::new(Access::new("token", vec![], None)), @@ -44,7 +55,12 @@ async fn serve(name: &str, remote: bool) -> Served { let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let port = listener.local_addr().unwrap().port(); tokio::spawn(async move { axum::serve(listener, app).await }); - Served { port, core, dir } + Served { + port, + core, + dir, + shell, + } } impl Drop for Served { @@ -53,9 +69,7 @@ impl Drop for Served { } } -fn password() -> String { - ["correct", "horse", "battery"].join(" ") -} +const NONCE: &str = "0123456789abcdef0123456789abcdef"; impl Served { async fn call( @@ -79,37 +93,31 @@ impl Served { (status, serde_json::from_str(&text).unwrap_or(Value::Null)) } - /// A private account, not one of the stand-in's seeded ones, whose password is public. - async fn sign_in(&self) { - self.core - .account - .start_sign_up( - "K7QM2XWH9QXA3C2D", - "crew-test@example.org", - Password::new(password()), - ) - .await - .unwrap(); - self.core - .account - .verify_email_code("ABCDEFGH") - .await - .unwrap(); + /// One line from the shell and the service's answer. + fn shell_says(&self, line: &Value) -> Value { + assert!(self.core.presence_line(&line.to_string())); + self.shell + .as_ref() + .expect("a desktop shell") + .lock() + .unwrap() + .try_recv() + .expect("the service answered the shell") } - /// What the window does before approving: "Confirm it's you" for this hold. - async fn nonce_for(&self, hold: &str) -> String { - let (status, body) = self - .call( - "POST", - "/api/account/confirm", - json!({ "password": password(), "action": "approve_device", - "device_id": format!("{}{hold}", orbyn_core::crew::CONFIRM_PREFIX) }), - None, - ) - .await; - assert_eq!(status, 200); - body["nonce"].as_str().unwrap().to_owned() + /// What the desktop app does after the person clicks Approve: describe the hold, grant what was described. + fn grant(&self, hold: &str, nonce: &str) -> String { + let shown = self.shell_says( + &json!({ "type": "presence_describe", "request_id": "r-1", "hold_id": hold }), + ); + assert_eq!(shown["type"], "presence_hold", "{shown}"); + let ready = self.shell_says(&json!({ + "type": "presence_grant", "request_id": "r-2", "nonce": nonce, "hold_id": hold, + "thread_id": shown["thread_id"], "task_id": shown["task_id"], + "command_sha256": shown["command_sha256"], + })); + assert_eq!(ready["type"], "presence_ready", "{ready}"); + nonce.to_owned() } /// A stored task with a worker, holding `command`: the hold's id. @@ -164,10 +172,9 @@ impl Served { } #[tokio::test] -async fn a_person_approves_once_with_the_confirmation_and_it_is_used_up() { +async fn a_person_approves_once_with_the_desktop_grant_and_it_is_used_up() { let app = serve("approve", false).await; let (_, hold) = app.held("a", "make deploy"); - app.sign_in().await; let path = format!("/api/crew/approvals/{hold}/approve-once"); let (status, body) = app @@ -177,30 +184,30 @@ async fn a_person_approves_once_with_the_confirmation_and_it_is_used_up() { assert_eq!(body["items"][0]["state"], "held"); assert_eq!(body["items"][0]["command"], "make deploy"); - // The service token alone is not enough: no confirmation, a wrong one, and one for another hold. + // The service token alone is not enough: no nonce, a wrong one, and one granted for another hold. let (status, body) = app.call("POST", &path, json!({}), None).await; - assert_eq!((status, &body["code"]), (403, &json!("reauth_required"))); + assert_eq!((status, &body["code"]), (403, &json!("presence_required"))); + let (status, body) = app.call("POST", &path, json!({}), Some(NONCE)).await; + assert_eq!((status, &body["code"]), (403, &json!("presence_required"))); + let (_, other) = app.held("b", "make ship"); + let for_other = app.grant(&other, "fedcba9876543210fedcba9876543210"); + let (status, body) = app.call("POST", &path, json!({}), Some(&for_other)).await; + assert_eq!((status, &body["code"]), (403, &json!("presence_required"))); + assert_eq!(app.state(&hold), "held", "nothing was approved"); + + // A wrong nonce does not burn the grant. + let nonce = app.grant(&hold, NONCE); let (status, _) = app .call("POST", &path, json!({}), Some("not-a-nonce")) .await; assert_eq!(status, 403); - let (_, other) = app.held("b", "make ship"); - let for_other = app.nonce_for(&other).await; - let (status, _) = app.call("POST", &path, json!({}), Some(&for_other)).await; - assert_eq!(status, 403); - assert_eq!(app.state(&hold), "held", "nothing was approved"); - - let nonce = app.nonce_for(&hold).await; let (status, body) = app.call("POST", &path, json!({}), Some(&nonce)).await; assert_eq!((status, &body["state"]), (200, &json!("approved"))); assert_eq!(body["command"], "make deploy"); - // Used up: the same nonce again, and a fresh one on a decided hold. - let (status, _) = app.call("POST", &path, json!({}), Some(&nonce)).await; - assert_eq!(status, 403); - let fresh = app.nonce_for(&hold).await; - let (status, body) = app.call("POST", &path, json!({}), Some(&fresh)).await; - assert_eq!((status, &body["code"]), (409, &json!("invalid_transition"))); + // Used up: the same nonce again fails, and the hold is decided. + let (status, body) = app.call("POST", &path, json!({}), Some(&nonce)).await; + assert_eq!((status, &body["code"]), (403, &json!("presence_required"))); let (_, log) = app .call( @@ -215,7 +222,56 @@ async fn a_person_approves_once_with_the_confirmation_and_it_is_used_up() { } #[tokio::test] -async fn deny_and_dismiss_need_no_password_and_are_decided_once() { +async fn a_grant_made_before_a_deny_does_not_approve_and_the_deny_stands() { + let app = serve("race", false).await; + let (_, hold) = app.held("a", "make deploy"); + let nonce = app.grant(&hold, NONCE); + let (status, _) = app + .call( + "POST", + &format!("/api/crew/approvals/{hold}/deny"), + json!({}), + None, + ) + .await; + assert_eq!(status, 200); + let (status, body) = app + .call( + "POST", + &format!("/api/crew/approvals/{hold}/approve-once"), + json!({}), + Some(&nonce), + ) + .await; + assert_eq!((status, &body["code"]), (409, &json!("invalid_transition"))); + assert_eq!(app.state(&hold), "denied"); + + // The other order: approved first, the deny then loses. + let (_, hold) = app.held("b", "make ship"); + let nonce = app.grant(&hold, NONCE); + let (status, _) = app + .call( + "POST", + &format!("/api/crew/approvals/{hold}/approve-once"), + json!({}), + Some(&nonce), + ) + .await; + assert_eq!(status, 200); + let (status, body) = app + .call( + "POST", + &format!("/api/crew/approvals/{hold}/deny"), + json!({}), + None, + ) + .await; + assert_eq!((status, &body["code"]), (409, &json!("invalid_transition"))); + assert_eq!(app.state(&hold), "approved"); +} + +#[tokio::test] +async fn deny_and_dismiss_need_no_grant_and_are_decided_once() { let app = serve("deny", false).await; let (_, one) = app.held("a", "make deploy"); let (_, two) = app.held("b", "make ship"); @@ -273,12 +329,11 @@ async fn deny_and_dismiss_need_no_password_and_are_decided_once() { async fn a_hold_whose_task_ended_answers_gone() { let app = serve("gone", false).await; let (task, hold) = app.held("a", "make deploy"); - app.sign_in().await; + let nonce = app.grant(&hold, NONCE); app.core .store .move_crew_task(&task, "cancelled", None) .unwrap(); - let nonce = app.nonce_for(&hold).await; let (status, body) = app .call( "POST", @@ -300,18 +355,24 @@ async fn a_hold_whose_task_ended_answers_gone() { } #[tokio::test] -async fn nothing_can_be_approved_without_a_signed_in_account() { - let app = serve("noaccount", false).await; +async fn a_service_without_a_desktop_shell_approves_nothing_with_or_without_the_token() { + let app = serve_with("noshell", false, false).await; let (_, hold) = app.held("a", "make deploy"); - let (status, _) = app - .call( - "POST", - &format!("/api/crew/approvals/{hold}/approve-once"), - json!({}), - Some("0".repeat(32).as_str()), - ) - .await; - assert_eq!(status, 403); + for nonce in [None, Some(NONCE)] { + let (status, body) = app + .call( + "POST", + &format!("/api/crew/approvals/{hold}/approve-once"), + json!({}), + nonce, + ) + .await; + assert_eq!((status, &body["code"]), (403, &json!("desktop_required"))); + } + // A grant cannot even be made: the line is read, and the answer has nowhere to go. + assert!(app.core.presence_line( + &json!({ "type": "presence_describe", "request_id": "r-1", "hold_id": hold }).to_string() + )); assert_eq!(app.state(&hold), "held"); } @@ -329,14 +390,15 @@ async fn a_remote_viewer_can_read_the_holds_and_answer_none() { "POST", &format!("/api/crew/approvals/{hold}/{verb}"), json!({}), - Some("n"), + Some(NONCE), ) .await; - assert_eq!( - (status, &body["code"]), - (403, &json!("local_only")), - "{verb}" - ); + let code = if verb == "approve-once" { + "desktop_required" + } else { + "local_only" + }; + assert_eq!((status, &body["code"]), (403, &json!(code)), "{verb}"); } assert_eq!(app.state(&hold), "held"); } diff --git a/crates/orbynd/src/routes/crew_tests.rs b/crates/orbynd/src/routes/crew_tests.rs index 0074ec99..55ea6832 100644 --- a/crates/orbynd/src/routes/crew_tests.rs +++ b/crates/orbynd/src/routes/crew_tests.rs @@ -232,7 +232,13 @@ async fn a_remote_viewer_reads_and_cannot_change_anything() { for (method, path) in STATE_CHANGING { let (status, body) = app.call(method, &concrete(path), Some(json!({}))).await; assert_eq!(status, 403, "a remote viewer cannot change state"); - assert_eq!(body["code"], "local_only"); + // Approving once needs the desktop app, so a remote viewer is told that; everything else is local only. + let code = if path.ends_with("/approve-once") { + "desktop_required" + } else { + "local_only" + }; + assert_eq!(body["code"], code, "{path}"); } let (status, _) = app.call("GET", "/api/crew/autopilot", None).await; assert_eq!(status, 200); diff --git a/crates/orbynd/tests/presence_stdio.rs b/crates/orbynd/tests/presence_stdio.rs new file mode 100644 index 00000000..0394c1f4 --- /dev/null +++ b/crates/orbynd/tests/presence_stdio.rs @@ -0,0 +1,121 @@ +//! The desktop shell's presence messages through the real binary's stdin and stdout, and the approve-once route's two refusals +//! (`desktop_required`, `presence_required`). Creating a hold needs a running crew task, which a spawned service cannot be given +//! here; the whole approval, with a hold, is in `routes/crew_held_tests.rs` and the core's `autopilot_held` tests. +//! This file checks what only a real process shows: the lines are read from stdin, answered on stdout, and a service started +//! without `--parent-stdin` has no shell to approve with. + +mod support; + +use std::io::{BufRead, Write}; + +use serde_json::{Value, json}; +use support::{Options, Service, TempDir}; + +fn options(desktop: bool, preview: bool) -> Options { + Options { + args: if desktop { + vec!["--parent-stdin".into()] + } else { + vec![] + }, + env: if preview { + vec![("ORBYN_CREW_PREVIEW".into(), "1".into())] + } else { + vec![] + }, + } +} + +fn say(service: &mut Service, line: &Value) { + let stdin = service.child.stdin.as_mut().unwrap(); + writeln!(stdin, "{line}").unwrap(); + stdin.flush().unwrap(); +} + +/// The next presence line the service wrote for the shell; other lines (activity) are passed over. +fn answer(service: &mut Service) -> Value { + loop { + let mut line = String::new(); + assert!( + service.stdout.read_line(&mut line).unwrap() > 0, + "the service closed its output" + ); + let value: Value = serde_json::from_str(&line).unwrap(); + if value["type"] + .as_str() + .is_some_and(|t| t.starts_with("presence_")) + { + return value; + } + } +} + +#[tokio::test] +async fn the_shell_is_answered_on_stdout_and_a_missing_hold_is_refused() { + let dir = TempDir::new(); + let mut service = Service::start(&dir.path().join("data"), options(true, true)); + say( + &mut service, + &json!({ "type": "presence_describe", "request_id": "r-1", "hold_id": "hold-nope" }), + ); + let said = answer(&mut service); + assert_eq!( + said, + json!({ "type": "presence_refused", "request_id": "r-1", "code": "not_found" }) + ); + say( + &mut service, + &json!({ "type": "presence_grant", "request_id": "r-2", "nonce": "short", "hold_id": "hold-nope", + "thread_id": "t", "task_id": "k", "command_sha256": "x" }), + ); + assert_eq!(answer(&mut service)["code"], "invalid"); + say( + &mut service, + &json!({ "type": "presence_grant", "request_id": "r-3", "nonce": "0123456789abcdef0123456789abcdef", + "hold_id": "hold-nope", "thread_id": "t", "task_id": "k", "command_sha256": "x" }), + ); + assert_eq!(answer(&mut service)["code"], "not_found"); + + // The token alone, with no nonce: a desktop service answers presence_required. + let response = service + .post("/api/crew/approvals/hold-nope/approve-once") + .json(&json!({})) + .send() + .await + .unwrap(); + assert_eq!(response.status(), 403); + let body: Value = response.json().await.unwrap(); + assert_eq!(body["code"], "presence_required"); + service.stop().await; +} + +#[tokio::test] +async fn without_the_preview_flag_the_shell_is_told_it_is_unavailable() { + let dir = TempDir::new(); + let mut service = Service::start(&dir.path().join("data"), options(true, false)); + say( + &mut service, + &json!({ "type": "presence_describe", "request_id": "r-1", "hold_id": "hold-nope" }), + ); + assert_eq!(answer(&mut service)["code"], "unavailable"); + service.stop().await; +} + +#[tokio::test] +async fn a_service_started_without_a_shell_answers_desktop_required() { + let dir = TempDir::new(); + let service = Service::start(&dir.path().join("data"), options(false, true)); + for nonce in [None, Some("0123456789abcdef0123456789abcdef")] { + let mut request = service + .post("/api/crew/approvals/hold-nope/approve-once") + .json(&json!({})); + if let Some(nonce) = nonce { + request = request.header("x-orbyn-confirm", nonce); + } + let response = request.send().await.unwrap(); + assert_eq!(response.status(), 403); + let body: Value = response.json().await.unwrap(); + assert_eq!(body["code"], "desktop_required", "{nonce:?}"); + } + service.stop().await; +} diff --git a/docs/architecture.md b/docs/architecture.md index 6b62b507..97a18ff7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -2218,6 +2218,22 @@ memory changed ([Instant reads](#instant-reads)). `ShellBridge` owns this Core c most, for the first window; `GET /api/shell` counts attached windows. Lines belonging to app updates continue to `UpdateCenter`. +Approving a held Crew command needs the person's click in the desktop app's own dialog +(docs/crew.md, "Approving once: local presence"). Over this channel the shell sends +`presence_describe {request_id, hold_id}` and `presence_grant {request_id, nonce, hold_id, +thread_id, task_id, command_sha256}`; the service answers on stdout `presence_hold` +(the redacted command, worker name, task title, ids and the hold's binding hash, all from +the service), `presence_ready` or `presence_refused {code}`. `host::presence::PresenceBook` +keeps one in-memory grant per hold for 60 seconds, bound to the hold, thread, task and hash, +replaced by a later grant, used once and dropped by a restart; `autopilot_approve_once` checks it +first without using it, runs its other checks, then takes it. A service without `--parent-stdin` +(or a remote one) answers `403 desktop_required`; a missing, wrong, used, expired or mismatched +nonce in `x-orbyn-confirm` is `403 presence_required`. No caller that only speaks HTTP to the +service can approve once, with or without the service token. This is not OS-level presence: a +process of the same OS user can still drive the native dialog (Accessibility, SendInput), read the +shell's memory or write into its pipe. Not built yet: the shell's dialog (PR2) and the web card +(PR3), so until then nothing mints a grant. + Only a service started with `--parent-stdin` accepts cross-origin native API traffic, from the two exact product asset origins to a loopback host. Preflight permits only its explicit methods and request headers. Actual requests still require the per-launch diff --git a/docs/crew.md b/docs/crew.md index 1bc92378..0a6f22aa 100644 --- a/docs/crew.md +++ b/docs/crew.md @@ -559,13 +559,34 @@ app's window, local only: `POST /api/crew/approvals/{hold}/approve-once`, `/deny racing approve and deny have exactly one winner. A hold that expired, or whose task ended, answers 410 (`gone`); one already decided answers 409. - **Never the always-denied list.** A command on the list (N1 to N10) is never held. The list is read again when the person approves and again when the worker retries, so a change in between (the checkout moved to main, say) turns the approval into a refusal. A person's deny rule still wins at the gate. -- **Proof that a person is there.** Approving needs the account's "Confirm it's you" for this hold: the window asks `POST /api/account/confirm` with the - action `approve_device` and the `device_id` `crew-approve-once:`, and sends the one-time nonce back in `x-orbyn-confirm`. It is for this hold - only, used up by its first use, 10 minutes at most, in memory only (the account's rules, [issue #435](account.md#sensitive-account-actions)). **It - protects against** a caller that holds only the service token (a worker, [#441](https://github.com/orbyn-dev/orbyn-engineering/issues/441)): it has no - nonce, so it cannot approve. **It does not protect against** a worker that can read the password or drive the window (the sandbox, #441), and it needs a - signed-in account: without one nothing can be approved once. **In release builds approve-once always fails with `account_disabled` (the account is off). In dev builds the stand-in account's password is public, so a worker holding the token could call `/api/account/confirm` itself (by a script N1 does not read) and approve its own hold: the protection is only as strong as the account password.** As a code guard, approve-once is refused while a seeded stand-in account (an `@example.com` address on the development backend) is signed in. Deny and dismiss need no password because they only refuse (their ledger rows say `decided_by: local_caller`, not `person`), so a token holder can deny or - dismiss a hold, never approve one. A dedicated confirmation action instead of the `approve_device` id convention waits for the account's own code to change. +- **Proof that a person is there: local presence (Approving once: local presence).** Approving needs a grant the desktop app records after the person + clicks Approve in the app's own native dialog; the account is not involved, so approve-once never answers `account_disabled`. The desktop shell launches + the service with `--parent-stdin` and a piped stdin only the shell holds (engines get stdin null). The flow, over newline JSON on that channel: + 1. The shell sends `presence_describe {request_id, hold_id}`. The service answers `presence_hold {request_id, hold_id, thread_id, task_id, + command_shown, command_sha256, worker_name, task_title}` or `presence_refused {request_id, code}`. The command, the worker's name and the task's title + come from the service, redacted (the answer key's text withheld) and bounded, never from the web page. `command_sha256` is the hold's binding hash. + 2. The shell shows the dialog. When the person clicks Approve it mints a nonce (16 random bytes, hex) and sends `presence_grant {request_id, nonce, + hold_id, thread_id, task_id, command_sha256}`. The service records one grant for the hold in memory (`PresenceBook`), only if the hold is open now + and every bound field matches it as it stands now, and answers `presence_ready {request_id}`. The shell hands the nonce to the page only after that. + A later grant for the same hold replaces the earlier one. A grant lives 60 seconds, is used by one approval and is cleared by a restart. + 3. The page posts `POST /api/crew/approvals/{hold}/approve-once` with the nonce in `x-orbyn-confirm`. The service checks the grant first, without + using it (constant-time compare), then the hold is open, its task is running, the command is readable and the always-denied list again; only then + does it use the grant up and decide. A refusal at any step leaves the grant as it was; a wrong nonce does not burn it. The ledger row is + `approved_once` with `via: "desktop_presence"` and `decided_by: "person"`. + Refusal codes on the shell channel: `unavailable`, `invalid`, `not_found`, `gone`, `unreadable`, `mismatch`. + **Errors on the route:** a service started without `--parent-stdin`, or a remote one, answers `403 desktop_required`; with a desktop shell, a missing, + wrong, used, expired or mismatched nonce is `403 presence_required`. `Origin: tauri://localhost` is not trusted for any of this. + **What it protects.** No caller that only speaks HTTP to the service can approve once, with or without the service token. Deny and dismiss need no grant + because they only refuse (their ledger rows say `decided_by: local_caller`, not `person`), so a token holder can deny or dismiss a hold, never approve one. + **What a process of the same OS user can still do, so this is not OS-level presence:** + - drive the desktop app's native dialog through Accessibility or SendInput, including a lead that is not confined; + - read the shell's memory (on macOS the hardened runtime blocks `task_for_pid`, but signing is not checked here; on Windows `PROCESS_VM_READ`); + - write into the shell's pipe to the service (on Linux `/proc//fd/0`, on Windows `DuplicateHandle`). + The devtools feature is on in release builds (`desktop/src-tauri/Cargo.toml:15`): a script in the page can call `invoke` but cannot click the native + dialog. That is a known exposure, tracked in [issue #532](https://github.com/orbyn-dev/orbyn-engineering/issues/532). + **Idea, not built (Linux only):** a per-launch HMAC key between the shell and the service, so a line written into the pipe by another process fails. + **Not built yet:** the shell's dialog (PR2) and the web card that asks for it and sends the nonce (PR3). Until the dialog exists nothing can mint a grant, + so approve-once answers `presence_required` (desktop service) or `desktop_required` (no shell) for every caller. - **N1 and remote viewers.** The three routes are in the Crew marker, so an agent's request to one is refused by path in every mode (the lead is never asked), a remote viewer is refused, and with the Preview flag off the routes answer 404 for every method. - **Ledger.** Every step writes a row: `held`, `approved_once`, `denied_by_person`, `dismissed_by_person`, `hold_used` (the retry went through; the From 2c270246cd5e864dcb9253226c147dc7ecb9601b Mon Sep 17 00:00:00 2001 From: Tran Duc Anh Dang Date: Fri, 13 Nov 2026 03:00:32 +1100 Subject: [PATCH 2/2] fix(crew): make the approval dialog text plain and test the nonce race presence_hold strings lose newlines and invisible or bidi characters (visible marks), the command says if it was cut, stale account-flow comments go, and two approvals with one nonce have one winner. --- .../orbyn-core/src/crew/autopilot/README.md | 2 +- crates/orbyn-core/src/crew/autopilot/hard.rs | 2 +- .../orbyn-core/src/crew/autopilot/presence.rs | 110 +++++++++++++++++- .../src/crew/tests/autopilot_held.rs | 69 ++++++++++- docs/crew.md | 6 +- 5 files changed, 178 insertions(+), 11 deletions(-) diff --git a/crates/orbyn-core/src/crew/autopilot/README.md b/crates/orbyn-core/src/crew/autopilot/README.md index ca550347..d87fa693 100644 --- a/crates/orbyn-core/src/crew/autopilot/README.md +++ b/crates/orbyn-core/src/crew/autopilot/README.md @@ -48,7 +48,7 @@ when the Crew Preview flag is on). For a Crew worker or reviewer: 5b. **Held for a person:** a denied command is held ([`held`](held.rs)). The person may approve that exact command once (bound to its text, arguments, paths, thread and task; one retry; 10 minutes), deny it or dismiss it, from the app's window only. The retry is checked against the always-denied list first and then lets the one approval through, with a ledger row (`decided_by: person`). Approving needs - the account's "Confirm it's you" for the hold. Nothing on the always-denied list is ever held. + the person's click in the desktop app's own dialog (`presence`). Nothing on the always-denied list is ever held. 6. **Trip wires** after each denial: 3 in a row or 20 in a run stop the task, `too_many_denials`. "In a run" is counted per task (a task has one run). Only an approval by the lead or by two keys ends a row of denials: a read that needs nobody does not, so a worker cannot put `ls` between its denials. diff --git a/crates/orbyn-core/src/crew/autopilot/hard.rs b/crates/orbyn-core/src/crew/autopilot/hard.rs index 41cb52ab..e5df694f 100644 --- a/crates/orbyn-core/src/crew/autopilot/hard.rs +++ b/crates/orbyn-core/src/crew/autopilot/hard.rs @@ -223,7 +223,7 @@ pub fn account_change(text: &str) -> bool { /// N1: a request to a state-changing account route, or to one this module does not know, and likewise a state-changing Crew /// route. **No agent approves; a person may approve once:** the routes that answer a held command (`approve-once`, `deny`, /// `dismiss`) are Crew routes that change state, so an agent's request to one is refused here, in every mode, and the person's -/// own call needs the account's confirmation (see `held`). Nothing on this list is ever held, and a person's approval cannot +/// own call needs the desktop app's grant (see `held`). Nothing on this list is ever held, and a person's approval cannot /// cover it: it is checked again at approval time. fn n1(ctx: &Ctx<'_>) -> bool { account_refused(ctx.kind, &ctx.target, ctx.paths) diff --git a/crates/orbyn-core/src/crew/autopilot/presence.rs b/crates/orbyn-core/src/crew/autopilot/presence.rs index 1a6dca2d..9d3db069 100644 --- a/crates/orbyn-core/src/crew/autopilot/presence.rs +++ b/crates/orbyn-core/src/crew/autopilot/presence.rs @@ -5,7 +5,7 @@ //! //! | The shell sends | The service answers on stdout | //! | --- | --- | -//! | `presence_describe {request_id, hold_id}` | `presence_hold {request_id, hold_id, thread_id, task_id, command_shown, command_sha256, worker_name, task_title}` or `presence_refused {request_id, code}` | +//! | `presence_describe {request_id, hold_id}` | `presence_hold {request_id, hold_id, thread_id, task_id, command_shown, command_sha256, worker_name, task_title, truncated}` or `presence_refused {request_id, code}` | //! | `presence_grant {request_id, nonce, hold_id, thread_id, task_id, command_sha256}` | `presence_ready {request_id}` or `presence_refused {request_id, code}` | //! //! The command, the worker's name and the task's title all come from here, redacted and bounded, never from the window: the @@ -35,16 +35,48 @@ struct Facts { thread: String, task: String, shown: String, + truncated: bool, bound: String, worker: String, title: String, } -/// Text for the dialog: redacted, the answer key's text withheld, cut to `max` characters, and empty if a secret or the key's -/// text would still show. +/// A character that draws nothing or changes how the text around it draws: controls, soft hyphen, zero-width and bidi marks, line +/// and paragraph separators, invisible operators, Hangul fillers, variation selectors, the byte order mark, tag characters. +fn invisible(c: char) -> bool { + c.is_control() + || matches!(c, + '\u{00AD}' | '\u{034F}' | '\u{061C}' | '\u{115F}' | '\u{1160}' | '\u{17B4}' | '\u{17B5}' + | '\u{180B}'..='\u{180F}' | '\u{200B}'..='\u{200F}' | '\u{2028}'..='\u{202E}' + | '\u{2060}'..='\u{206F}' | '\u{3164}' | '\u{FE00}'..='\u{FE0F}' | '\u{FEFF}' | '\u{FFA0}' + | '\u{FFF9}'..='\u{FFFC}' | '\u{1BCA0}'..='\u{1BCA3}' | '\u{1D173}'..='\u{1D17A}' + | '\u{E0000}'..='\u{E0FFF}') +} + +/// Text a dialog can show as plain text without it faking a line or reordering: a newline becomes a visible `⏎` and any other +/// invisible character a visible `\u{FFFD}`. One character in, one character out. +fn plain(text: &str) -> String { + text.chars() + .map(|c| match c { + '\n' | '\r' => '\u{23CE}', + c if invisible(c) => '\u{FFFD}', + c => c, + }) + .collect() +} + +/// `text` cut to `max` characters, and whether it was cut. +fn capped(text: &str, max: usize) -> (String, bool) { + let cut: String = text.chars().take(max).collect(); + let was_cut = text.chars().nth(max).is_some(); + (cut, was_cut) +} + +/// Text for the dialog: redacted, the answer key's text withheld, made plain, cut to `max` characters, and empty if a secret or +/// the key's text would still show. fn bounded(text: &str, max: usize, needles: &[String]) -> String { - let clean = crate::crew::withhold(&crate::crew::redact(text), needles); - let cut: String = clean.chars().take(max).collect(); + let clean = plain(&crate::crew::withhold(&crate::crew::redact(text), needles)); + let (cut, _) = capped(&clean, max); let value = json!(cut); if crate::crew::refuse_secret(&value).is_ok() && crate::crew::refuse_leak(&value, needles).is_ok() @@ -103,6 +135,7 @@ impl Core { "thread_id": facts.thread, "task_id": facts.task, "command_shown": facts.shown, + "truncated": facts.truncated, "command_sha256": facts.bound, "worker_name": facts.worker, "task_title": facts.title, @@ -170,6 +203,7 @@ impl Core { if !held.readable { return Err("unreadable"); } + let (shown, truncated) = capped(&plain(&held.shown), crate::crew::SHOWN_MAX); let needles = self.held_needles(&held.task_id); let title = self .store @@ -182,10 +216,74 @@ impl Core { hold: held.id.clone(), thread: held.thread_id.clone(), task: held.task_id.clone(), - shown: held.shown.clone(), + shown, + truncated, bound: held.bound.clone(), worker: bounded(&run.member, WORKER_NAME_MAX, &needles), title: bounded(&title, TASK_TITLE_MAX, &needles), }) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn newlines_become_a_visible_mark_and_invisible_characters_a_placeholder() { + assert_eq!(plain("a\nb\r\nc"), "a\u{23CE}b\u{23CE}\u{23CE}c"); + for c in [ + '\u{0}', + '\u{7}', + '\u{1B}', + '\u{85}', + '\u{9F}', + '\t', + '\u{00AD}', + '\u{200B}', + '\u{200E}', + '\u{200F}', + '\u{2028}', + '\u{2029}', + '\u{202A}', + '\u{202E}', + '\u{2066}', + '\u{2067}', + '\u{2069}', + '\u{2060}', + '\u{3164}', + '\u{FE0F}', + '\u{FEFF}', + '\u{E0041}', + '\u{FFA0}', + ] { + assert_eq!(plain(&format!("a{c}b")), "a\u{FFFD}b", "U+{:04X}", c as u32); + } + // Ordinary text, including other scripts and emoji, is left as it is. + let text = "make deploy --to \"é\" 日本語 \u{1F680} ~/x"; + assert_eq!(plain(text), text); + } + + #[test] + fn a_title_cannot_fake_a_line_or_reorder_the_dialog() { + let title = "Fix the bug\nCommand: rm -rf ~\u{202E}\u{2066}x"; + let shown = bounded(title, TASK_TITLE_MAX, &[]); + assert!(!shown.chars().any(invisible), "{shown:?}"); + assert!(!shown.contains('\n')); + assert!(shown.starts_with("Fix the bug\u{23CE}Command")); + let name = bounded("worker\u{200B}\n01", WORKER_NAME_MAX, &[]); + assert_eq!(name, "worker\u{FFFD}\u{23CE}01"); + } + + #[test] + fn text_is_cut_at_the_limit_and_says_so() { + assert_eq!(capped("abc", 3), ("abc".to_owned(), false)); + assert_eq!(capped("abcd", 3), ("abc".to_owned(), true)); + assert_eq!( + bounded(&"x".repeat(500), TASK_TITLE_MAX, &[]) + .chars() + .count(), + TASK_TITLE_MAX + ); + } +} diff --git a/crates/orbyn-core/src/crew/tests/autopilot_held.rs b/crates/orbyn-core/src/crew/tests/autopilot_held.rs index 6d8b2bad..756eb77d 100644 --- a/crates/orbyn-core/src/crew/tests/autopilot_held.rs +++ b/crates/orbyn-core/src/crew/tests/autopilot_held.rs @@ -1,5 +1,5 @@ -//! A person's one-time answer to a held command, through a real `Core`, the real gate and the account's "Confirm it's you" -//! (the development stand-in for the Engine). The lead says no, so a command is denied and held; the person then approves it +//! A person's one-time answer to a held command, through a real `Core`, the real gate and a played desktop shell (the +//! presence messages of `autopilot::presence`). The lead says no, so a command is denied and held; the person then approves it //! once, denies it or dismisses it. Values that stand for a secret are built at run time and named by label. use orbyn_contracts::{ActionKind, AutopilotHeldState as State, StopReason}; @@ -738,3 +738,68 @@ async fn with_the_flag_off_none_of_it_exists() { Err(CoreError::NotFound(_)) )); } + +#[tokio::test] +async fn the_dialog_text_has_no_newline_control_or_bidi_character() { + let (w, id) = held_world().await; + let command = "make deploy\u{202E}\n\u{200B}check\u{2028}\u{3164}x"; + // The always-denied list may well read such a command itself; the hold is made directly, as the chain's denial would. + let task = w.core.store.crew_task(&id).unwrap().unwrap(); + let hold = w + .core + .held_make( + &task, + &w.worker(&id), + &ask(ActionKind::Command, command, &[]), + "The lead said no.", + &[], + unix_now(), + ) + .expect("held"); + let shown = describe(&w, &hold, unix_now()); + assert_eq!(shown["type"], "presence_hold", "{shown}"); + let text = shown["command_shown"].as_str().unwrap(); + assert!( + text.contains('\u{23CE}') && text.contains('\u{FFFD}'), + "{text:?}" + ); + for field in ["command_shown", "worker_name", "task_title"] { + let value = shown[field].as_str().unwrap(); + assert!( + !value + .chars() + .any(|c| c.is_control() || ('\u{200B}'..='\u{202E}').contains(&c)), + "{field}: {value:?}" + ); + } + assert_eq!(shown["truncated"], false); + // The hash is of the real command, not of what is shown. + assert_eq!(shown["command_sha256"].as_str().unwrap().len(), 64); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn two_approvals_with_the_same_nonce_racing_have_exactly_one_success() { + for round in 0..10 { + let (w, id) = held_world().await; + let hold = hold_of(&w, &id, &format!("make step-{round}")).await; + let nonce = grant_for(&w, &hold); + let mut tasks = Vec::new(); + for _ in 0..4 { + let (core, hold, nonce) = (w.core.clone(), hold.clone(), nonce.clone()); + tasks.push(tokio::spawn(async move { + core.autopilot_approve_once(&hold, Some(&nonce)).await + })); + } + let mut wins = 0; + for task in tasks { + match task.await.unwrap() { + Ok(_) => wins += 1, + Err(CoreError::PresenceRequired | CoreError::InvalidTransition { .. }) => {} + Err(other) => panic!("{other:?}"), + } + } + assert_eq!(wins, 1, "round {round}"); + assert_eq!(events(&w, &id, "approved_once").len(), 1); + assert_eq!(w.core.presence.len(), 0); + } +} diff --git a/docs/crew.md b/docs/crew.md index 0a6f22aa..b3d756eb 100644 --- a/docs/crew.md +++ b/docs/crew.md @@ -561,7 +561,7 @@ app's window, local only: `POST /api/crew/approvals/{hold}/approve-once`, `/deny worker retries, so a change in between (the checkout moved to main, say) turns the approval into a refusal. A person's deny rule still wins at the gate. - **Proof that a person is there: local presence (Approving once: local presence).** Approving needs a grant the desktop app records after the person clicks Approve in the app's own native dialog; the account is not involved, so approve-once never answers `account_disabled`. The desktop shell launches - the service with `--parent-stdin` and a piped stdin only the shell holds (engines get stdin null). The flow, over newline JSON on that channel: + the service with `--parent-stdin` and a piped stdin only the shell holds (an engine gets its own piped stdin from the service, never the shell's). The flow, over newline JSON on that channel: 1. The shell sends `presence_describe {request_id, hold_id}`. The service answers `presence_hold {request_id, hold_id, thread_id, task_id, command_shown, command_sha256, worker_name, task_title}` or `presence_refused {request_id, code}`. The command, the worker's name and the task's title come from the service, redacted (the answer key's text withheld) and bounded, never from the web page. `command_sha256` is the hold's binding hash. @@ -573,6 +573,10 @@ app's window, local only: `POST /api/crew/approvals/{hold}/approve-once`, `/deny using it (constant-time compare), then the hold is open, its task is running, the command is readable and the always-denied list again; only then does it use the grant up and decide. A refusal at any step leaves the grant as it was; a wrong nonce does not burn it. The ledger row is `approved_once` with `via: "desktop_presence"` and `decided_by: "person"`. + The strings in `presence_hold` are made for a plain-text dialog: control, invisible, bidirectional and format characters become a visible `�`, + a newline becomes a visible `⏎`, the command is capped at 1000 characters with `truncated` set if it was cut (the hash is of the full command), and + a hold that cannot be read whole is refused with `unreadable`. **PR2's dialog must render all of them as plain text** (no markup, no HTML, no + link detection), and show them as given. Refusal codes on the shell channel: `unavailable`, `invalid`, `not_found`, `gone`, `unreadable`, `mismatch`. **Errors on the route:** a service started without `--parent-stdin`, or a remote one, answers `403 desktop_required`; with a desktop shell, a missing, wrong, used, expired or mismatched nonce is `403 presence_required`. `Origin: tauri://localhost` is not trusted for any of this.