From 60040fb9437f8dad6f03af41e6398463cc60fb0c Mon Sep 17 00:00:00 2001 From: Tran Duc Anh Dang Date: Thu, 12 Nov 2026 12:00:22 +1100 Subject: [PATCH 1/2] fix(device-client): harden the approver's reveal, paths and ended approvals A network failure during the reveal is retried (bounded) and never cancels the approval; only a real mismatch does. ApproverState's Debug hides the digits and the fingerprint. Ids in request paths are percent-encoded. confirm and deny on an ended approval give ApprovalExpired and never approve. The approver can read the time left from the expiry the server already returns. --- crates/orbyn-device-client/src/approvals.rs | 193 ++++++++- crates/orbyn-device-client/src/sync.rs | 2 +- crates/orbyn-device-client/tests/approvals.rs | 373 ++++++++++++++++++ crates/orbyn-server/tests/device_client.rs | 45 +++ docs/device-client.md | 26 ++ 5 files changed, 623 insertions(+), 16 deletions(-) create mode 100644 crates/orbyn-device-client/tests/approvals.rs diff --git a/crates/orbyn-device-client/src/approvals.rs b/crates/orbyn-device-client/src/approvals.rs index ede765e..1aaa93d 100644 --- a/crates/orbyn-device-client/src/approvals.rs +++ b/crates/orbyn-device-client/src/approvals.rs @@ -11,9 +11,15 @@ //! `deny_*`. Every approval is used once. use crate::client::{DeviceClient, json, parse}; +use crate::sync::segment; use crate::{Clock, DeviceApi, DeviceError, KeyStore, Method}; use orbyn_client::{ApproverPairing, ClientError, DevicePairing, Engine}; use orbyn_contracts as wire; +use std::time::Duration; + +/// How many times the reveal is sent before a network failure is reported. The approval is not +/// cancelled by a network failure: the person can ask again. +const REVEAL_TRIES: usize = 3; fn mismatch(error: ClientError) -> DeviceError { match error { @@ -29,6 +35,11 @@ fn mismatch(error: ClientError) -> DeviceError { pub struct ApproverApproval { pairing: ApproverPairing, revealed: bool, + /// The reveal the engine made, kept until the server has it: the engine makes it once, so a + /// network failure must be answered by sending this again, not by asking the engine again. + unsent_reveal: Option, + /// When the server will close the approval, Unix seconds, as it last said. + expires_at: Option, } impl ApproverApproval { @@ -37,8 +48,41 @@ impl ApproverApproval { } } -/// What the approver's screen shows. -#[derive(Clone, Debug, PartialEq, Eq)] +/// Unix seconds of an RFC 3339 time in UTC (`2026-11-12T01:02:03.456Z`, as the server writes it). +/// Any other offset is `None`: the caller then shows no countdown rather than a wrong one. +fn rfc3339_secs(text: &str) -> Option { + let text = text + .strip_suffix('Z') + .or_else(|| text.strip_suffix("+00:00"))?; + let (date, time) = text.split_once('T')?; + let mut d = date.split('-'); + let (year, month, day): (i64, i64, i64) = ( + d.next()?.parse().ok()?, + d.next()?.parse().ok()?, + d.next()?.parse().ok()?, + ); + let time = time.split('.').next()?; + let mut t = time.split(':'); + let (h, m, sec): (i64, i64, i64) = ( + t.next()?.parse().ok()?, + t.next()?.parse().ok()?, + t.next()?.parse().ok()?, + ); + if !(1..=12).contains(&month) || !(1..=31).contains(&day) || h > 23 || m > 59 || sec > 60 { + return None; + } + // Days since 1970-01-01 (the civil-from-days algorithm, inverted). + let y = if month <= 2 { year - 1 } else { year }; + let era = y.div_euclid(400); + let yoe = y - era * 400; + let doy = (153 * (if month > 2 { month - 3 } else { month + 9 }) + 2) / 5 + day - 1; + let doe = yoe * 365 + yoe / 4 - yoe / 100 + doy; + let days = era * 146_097 + doe - 719_468; + Some(days * 86_400 + h * 3600 + m * 60 + sec) +} + +/// What the approver's screen shows. Its `Debug` hides the digits and the fingerprint. +#[derive(Clone, PartialEq, Eq)] pub enum ApproverState { /// The new device has not joined yet. Waiting, @@ -50,6 +94,15 @@ pub enum ApproverState { }, } +impl std::fmt::Debug for ApproverState { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Waiting => f.write_str("Waiting"), + Self::Digits { .. } => f.write_str("Digits { .. }"), + } + } +} + /// An approval the new device joined. Use it once. pub struct JoinedApproval { pairing: DevicePairing, @@ -62,13 +115,20 @@ impl JoinedApproval { } impl DeviceClient { + /// The path of an approval or one of its steps. The id is percent-encoded: on the new + /// device's side it comes from the server. fn approval_path(id: &str, step: Option<&str>) -> String { + let id = segment(id); match step { Some(step) => format!("/v1/devices/approvals/{id}/{step}"), None => format!("/v1/devices/approvals/{id}"), } } + fn device_path(id: &str) -> String { + format!("/v1/devices/{}", segment(id)) + } + /// Best effort: a cancel that fails changes nothing, and the approval ends in ten minutes. async fn cancel_quietly(&self, id: &str) { let _ = self @@ -108,6 +168,8 @@ impl DeviceClient { Ok(ApproverApproval { pairing, revealed: false, + unsent_reveal: None, + expires_at: rfc3339_secs(&created.expires_at), }) } @@ -123,24 +185,25 @@ impl DeviceClient { .authed(Method::Get, &Self::approval_path(&id, None), None) .await?; let view: wire::PendingApproval = parse(&response.body)?; + approval.expires_at = rfc3339_secs(&view.expires_at).or(approval.expires_at); if view.device_nonce.is_none() { return Ok(ApproverState::Waiting); } if !approval.revealed { - let reveal = match approval.pairing.reveal(&view) { - Ok(reveal) => reveal, - Err(error) => { - self.cancel_quietly(&id).await; - return Err(mismatch(error)); - } + let reveal = match approval.unsent_reveal.clone() { + Some(reveal) => reveal, + None => match approval.pairing.reveal(&view) { + Ok(reveal) => reveal, + Err(error) => { + self.cancel_quietly(&id).await; + return Err(mismatch(error)); + } + }, }; - self.authed( - Method::Post, - &Self::approval_path(&id, Some("reveal")), - Some(json(&reveal)?), - ) - .await?; + approval.unsent_reveal = Some(reveal.clone()); + self.send_reveal(&id, &reveal).await?; approval.revealed = true; + approval.unsent_reveal = None; } let digits = approval .pairing @@ -152,6 +215,50 @@ impl DeviceClient { }) } + /// Sends the reveal, again if the network fails. A failure never cancels the approval: this + /// device's nonce may have arrived, so before sending again the server is asked whether it + /// holds it. Only an answer from the server that refuses the reveal ends the approval, and the + /// server does that itself. + async fn send_reveal(&self, id: &str, reveal: &wire::ApprovalNonce) -> Result<(), DeviceError> { + let mut last = DeviceError::Transport; + for _ in 0..REVEAL_TRIES { + let sent = self + .authed( + Method::Post, + &Self::approval_path(id, Some("reveal")), + Some(json(reveal)?), + ) + .await; + match sent { + Ok(_) => return Ok(()), + Err(DeviceError::Transport) => { + last = DeviceError::Transport; + let held = self + .authed(Method::Get, &Self::approval_path(id, None), None) + .await + .ok() + .and_then(|r| parse::(&r.body).ok()) + .is_some_and(|v| v.approver_nonce.as_deref() == Some(&reveal.nonce)); + if held { + return Ok(()); + } + } + Err(other) => return Err(other), + } + } + Err(last) + } + + /// How long the server will keep the approval open, from the time it gave when it opened it + /// (refreshed by [`DeviceClient::approval_state`]) and this device's clock. `None` when the + /// server's time could not be read. Zero once it passed: the server then answers + /// [`DeviceError::ApprovalExpired`]. A clock that is off by seconds shows a countdown that is + /// off by seconds; the server decides, not this. + pub fn time_left(&self, approval: &ApproverApproval) -> Option { + let left = approval.expires_at? - self.clock.now_secs(); + Some(Duration::from_secs(u64::try_from(left).unwrap_or(0))) + } + /// The person confirmed that both screens show the same digits: signs the approval with the /// devices this device trusts and sends it. Not before: [`DeviceError::ApprovalMismatch`] if /// the digits were never shown. The host then calls [`DeviceClient::send_keys`] for each zone @@ -177,6 +284,11 @@ impl DeviceClient { /// The person said the digits differ, or cancelled: ends the approval on the server. pub async fn deny_approval(&self, approval: ApproverApproval) -> Result<(), DeviceError> { + // An approval that already ended (expired, answered, used or cancelled by either device) + // is `ApprovalExpired` here: the server answers 410 to this read, nothing is sent to end + // it again and nothing is approved. + self.authed(Method::Get, &Self::approval_path(approval.id(), None), None) + .await?; self.authed( Method::Post, &Self::approval_path(approval.id(), Some("cancel")), @@ -314,10 +426,61 @@ impl DeviceClient { let body = engine.sign_removal(device).map_err(mismatch)?; self.authed( Method::Delete, - &format!("/v1/devices/{}", device.id), + &Self::device_path(&device.id), Some(json(&body)?), ) .await?; Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{HttpApi, MemoryStore, SystemClock}; + + type Client = DeviceClient; + + #[test] + fn ids_are_percent_encoded_in_every_path() { + assert_eq!( + Client::approval_path("a/b?c", Some("reveal")), + "/v1/devices/approvals/a%2Fb%3Fc/reveal" + ); + assert_eq!( + Client::approval_path("a/b?c", None), + "/v1/devices/approvals/a%2Fb%3Fc" + ); + assert_eq!(Client::device_path("a/b?c"), "/v1/devices/a%2Fb%3Fc"); + // A uuid is unchanged. + let id = "0000000a-0000-4000-8000-00000000000a"; + assert_eq!( + Client::approval_path(id, Some("nonce")), + format!("/v1/devices/approvals/{id}/nonce") + ); + } + + #[test] + fn the_server_time_is_read_as_utc_and_nothing_else_is_guessed() { + assert_eq!(rfc3339_secs("1970-01-01T00:00:00Z"), Some(0)); + assert_eq!( + rfc3339_secs("2027-01-15T08:00:00.123Z"), + Some(1_800_000_000) + ); + assert_eq!( + rfc3339_secs("2024-02-29T12:00:00+00:00"), + Some(1_709_208_000) + ); + assert_eq!(rfc3339_secs("1969-12-31T23:59:59Z"), Some(-1)); + for bad in [ + "2027-01-15T10:00:00+02:00", + "2027-01-15 08:00:00Z", + "2027-13-15T08:00:00Z", + "2027-01-15T25:00:00Z", + "garbage", + "", + ] { + assert_eq!(rfc3339_secs(bad), None, "{bad}"); + } + } +} diff --git a/crates/orbyn-device-client/src/sync.rs b/crates/orbyn-device-client/src/sync.rs index 417a7c5..6d44b81 100644 --- a/crates/orbyn-device-client/src/sync.rs +++ b/crates/orbyn-device-client/src/sync.rs @@ -52,7 +52,7 @@ pub(crate) fn engine_error(error: ClientError) -> DeviceError { } /// A path segment, percent-encoded: zone names and tokens may hold `:` or `.`. -fn segment(text: &str) -> String { +pub(crate) fn segment(text: &str) -> String { let mut out = String::with_capacity(text.len()); for byte in text.bytes() { match byte { diff --git a/crates/orbyn-device-client/tests/approvals.rs b/crates/orbyn-device-client/tests/approvals.rs new file mode 100644 index 0000000..56672e3 --- /dev/null +++ b/crates/orbyn-device-client/tests/approvals.rs @@ -0,0 +1,373 @@ +//! The approver's calls over a scripted transport: a network failure during the reveal, the time +//! left, the redacted `Debug`, and an approval that already ended. The real server's side of the +//! same steps is in `orbyn-server/tests/device_client.rs`. + +use base64::Engine as _; +use orbyn_device_client::{ + ApproverApproval, ApproverState, ClientConfig, Clock, DeviceApi, DeviceClient, DeviceError, + DeviceKeys, DevicePlatform, Engine, KeyStore, MemoryStore, Method, Request, Response, + StoredIdentity, StoredSession, Zeroizing, +}; +use serde_json::{Value, json}; +use std::sync::atomic::{AtomicI64, Ordering}; +use std::sync::{Arc, Mutex}; +use std::time::Duration; + +const ACCOUNT: &str = "0a0a0a0a-0000-4000-8000-000000000001"; +const APPROVER: &str = "0000000a-0000-4000-8000-00000000000a"; +const NEW: &str = "0000000c-0000-4000-8000-00000000000c"; +/// 2027-01-15T08:00:00Z. +const NOW: i64 = 1_800_000_000; +/// Ten minutes after `NOW`. +const EXPIRES: &str = "2027-01-15T08:10:00.000Z"; + +fn b64(bytes: &[u8]) -> String { + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes) +} + +#[derive(Clone, Copy, PartialEq, Eq)] +enum Reveal { + Works, + /// The network fails this many times before the request goes through. + NetworkFails(usize), + /// The request reaches the server and is kept, but the answer is lost, once. + AnswerLostOnce, +} + +struct State { + create: Option, + reveal: Reveal, + approver_nonce: Option, + /// The server says the approval ended: expired, answered, used or cancelled. + ended: bool, + expires_at: String, + approved: bool, + /// What the server shows as the commitment, when it lies about it. + commitment: Option, + log: Vec, +} + +struct Script { + state: Mutex, + now: Arc, + approver_keys: DeviceKeys, + new_keys: DeviceKeys, +} + +fn answer(status: u16, body: Value) -> Result { + let bytes = if body.is_null() { + Vec::new() + } else { + serde_json::to_vec(&body).expect("json") + }; + Ok(Response { + status, + retry_after: None, + body: Zeroizing::new(bytes), + }) +} + +fn gone() -> Result { + answer(410, json!({"code": "approval_expired", "message": "x"})) +} + +impl DeviceApi for Script { + async fn send(&self, request: Request) -> Result { + let mut s = self.state.lock().expect("lock"); + s.log.push(format!("{:?} {}", request.method, request.path)); + let body: Value = request + .body + .as_ref() + .and_then(|b| serde_json::from_slice(b).ok()) + .unwrap_or(Value::Null); + let path = request.path.clone(); + let step = path.rsplit('/').next().unwrap_or_default().to_owned(); + match (request.method, path.as_str()) { + (Method::Get, "/v1/devices") => answer( + 200, + json!({"devices": [{ + "id": NEW, "name": "Phone", "platform": "linux", "state": "pending", + "signing_key": b64(&self.new_keys.public().signing), + "agreement_key": b64(&self.new_keys.public().agreement), + "created_at": "2027-01-15T07:00:00Z", "approved_by": null, + "approval_signature": null, "current": false, + }], "removals": []}), + ), + (Method::Post, "/v1/devices/approvals") => { + s.create = Some(body.clone()); + let id = body["approval_id"].clone(); + answer(201, json!({"id": id, "expires_at": EXPIRES})) + } + (Method::Get, p) if p.starts_with("/v1/devices/approvals/") => { + if s.ended { + return gone(); + } + let create = s.create.clone().expect("created first"); + answer( + 200, + json!({ + "id": create["approval_id"], "expires_at": s.expires_at, + "created_at": create["created_at"], "approver_id": APPROVER, + "approver_signing_key": b64(&self.approver_keys.public().signing), + "approver_agreement_key": b64(&self.approver_keys.public().agreement), + "device_id": NEW, + "device_signing_key": b64(&self.new_keys.public().signing), + "device_agreement_key": b64(&self.new_keys.public().agreement), + "commitment": s.commitment.clone().map_or(create["commitment"].clone(), Value::from), + "device_nonce": b64(&[7u8; 32]), + "approver_nonce": s.approver_nonce, "signature": null, + "endorsement": null, + }), + ) + } + (Method::Post, _) if step == "reveal" => match s.reveal { + Reveal::NetworkFails(n) if n > 0 => { + s.reveal = Reveal::NetworkFails(n - 1); + Err(DeviceError::Transport) + } + Reveal::AnswerLostOnce => { + s.approver_nonce = body["nonce"].as_str().map(str::to_owned); + s.reveal = Reveal::Works; + Err(DeviceError::Transport) + } + _ => { + s.approver_nonce = body["nonce"].as_str().map(str::to_owned); + answer(204, Value::Null) + } + }, + (Method::Post, _) if step == "confirm" => { + if s.ended { + return gone(); + } + s.approved = true; + answer(204, Value::Null) + } + (Method::Post, _) if step == "cancel" => { + if s.ended { + return gone(); + } + s.ended = true; + answer(204, Value::Null) + } + _ => answer(404, json!({"code": "not_found", "message": "x"})), + } + } +} + +struct Fixed(Arc); + +impl Clock for Fixed { + fn now_secs(&self) -> i64 { + self.0.load(Ordering::SeqCst) + } +} + +type Client = DeviceClient; + +fn setup(reveal: Reveal) -> (Client, Engine) { + let keys = DeviceKeys::generate().expect("keys"); + let copy = DeviceKeys::from_bytes(keys.to_bytes().as_slice()).expect("copy"); + let new_keys = DeviceKeys::generate().expect("keys"); + let store = MemoryStore::new(); + store + .save(&StoredIdentity { + device_keys: keys.to_bytes(), + account_id: ACCOUNT.to_owned(), + device_id: APPROVER.to_owned(), + email: "a@example.test".to_owned(), + session: Some(StoredSession { + token: Zeroizing::new("token".to_owned()), + expires_at: "2028-01-01T00:00:00Z".to_owned(), + }), + vault_key: Zeroizing::new([5u8; 32]), + }) + .expect("save"); + let now = Arc::new(AtomicI64::new(NOW)); + let client = DeviceClient::new( + Script { + now: now.clone(), + state: Mutex::new(State { + create: None, + reveal, + approver_nonce: None, + ended: false, + expires_at: EXPIRES.to_owned(), + approved: false, + commitment: None, + log: Vec::new(), + }), + approver_keys: copy, + new_keys, + }, + store, + Fixed(now), + ClientConfig { + device_name: "Laptop".to_owned(), + platform: DevicePlatform::Linux, + app: None, + }, + ); + let engine = Engine::new(keys, ACCOUNT, APPROVER).expect("engine"); + (client, engine) +} + +fn log(client: &Client) -> Vec { + client.api().state.lock().expect("lock").log.clone() +} + +fn count(client: &Client, needle: &str) -> usize { + log(client).iter().filter(|l| l.contains(needle)).count() +} + +async fn started(client: &Client, engine: &Engine) -> ApproverApproval { + client.start_approval(engine, NEW).await.expect("start") +} + +#[tokio::test] +async fn a_network_error_during_the_reveal_is_retried_and_never_cancels() { + let (client, engine) = setup(Reveal::NetworkFails(2)); + let mut approval = started(&client, &engine).await; + let state = client.approval_state(&mut approval).await.expect("state"); + assert!(matches!(state, ApproverState::Digits { .. })); + assert_eq!( + count(&client, "reveal"), + 3, + "two failures, then it went through" + ); + assert_eq!(count(&client, "cancel"), 0); +} + +#[tokio::test] +async fn a_reveal_that_keeps_failing_reports_the_network_and_leaves_the_approval_open() { + let (client, engine) = setup(Reveal::NetworkFails(100)); + let mut approval = started(&client, &engine).await; + assert_eq!( + client.approval_state(&mut approval).await.err(), + Some(DeviceError::Transport) + ); + assert_eq!(count(&client, "reveal"), 3, "a bounded number of tries"); + assert_eq!( + count(&client, "cancel"), + 0, + "a network failure does not cancel" + ); + // The network is back: the same approval goes on. + client.api().state.lock().expect("lock").reveal = Reveal::Works; + let state = client.approval_state(&mut approval).await.expect("state"); + assert!(matches!(state, ApproverState::Digits { .. })); + assert_eq!(count(&client, "cancel"), 0); +} + +#[tokio::test] +async fn a_reveal_whose_answer_was_lost_is_not_sent_again_when_the_server_holds_it() { + let (client, engine) = setup(Reveal::AnswerLostOnce); + let mut approval = started(&client, &engine).await; + let state = client.approval_state(&mut approval).await.expect("state"); + assert!(matches!(state, ApproverState::Digits { .. })); + assert_eq!( + count(&client, "reveal"), + 1, + "the server already held the nonce" + ); + assert_eq!(count(&client, "cancel"), 0); +} + +#[tokio::test] +async fn the_digits_and_the_fingerprint_never_show_in_debug() { + let (client, engine) = setup(Reveal::Works); + let mut approval = started(&client, &engine).await; + let state = client.approval_state(&mut approval).await.expect("state"); + let ApproverState::Digits { + digits, + fingerprint, + } = &state + else { + panic!("digits"); + }; + for text in [format!("{state:?}"), format!("{state:#?}")] { + assert!(!text.contains(digits.as_str()), "{text}"); + assert!(!text.contains(fingerprint.as_str()), "{text}"); + assert!(text.contains("Digits"), "{text}"); + } + assert_eq!(format!("{:?}", ApproverState::Waiting), "Waiting"); +} + +#[tokio::test] +async fn the_time_left_comes_from_what_the_server_said_and_this_clock() { + let (client, engine) = setup(Reveal::Works); + let mut approval = started(&client, &engine).await; + assert_eq!(client.time_left(&approval), Some(Duration::from_secs(600))); + // Another 100 seconds pass on this device's clock. + client.api().now.store(NOW + 100, Ordering::SeqCst); + assert_eq!(client.time_left(&approval), Some(Duration::from_secs(500))); + // The server's later answer refreshes it. + client.api().state.lock().expect("lock").expires_at = "2027-01-15T08:20:00Z".to_owned(); + client.approval_state(&mut approval).await.expect("state"); + assert_eq!(client.time_left(&approval), Some(Duration::from_secs(1100))); + // Past the end it is zero, never negative. + client.api().now.store(NOW + 5000, Ordering::SeqCst); + assert_eq!(client.time_left(&approval), Some(Duration::ZERO)); +} + +#[tokio::test] +async fn an_unreadable_expiry_shows_no_countdown() { + let (client, engine) = setup(Reveal::Works); + let mut approval = started(&client, &engine).await; + // Neither an offset other than UTC nor garbage is guessed at; the last good time is kept. + client.api().state.lock().expect("lock").expires_at = "2027-01-15T10:10:00+02:00".to_owned(); + client.approval_state(&mut approval).await.expect("state"); + assert_eq!(client.time_left(&approval), Some(Duration::from_secs(600))); +} + +#[tokio::test] +async fn confirm_and_deny_on_an_approval_that_already_ended_never_approve() { + // Confirm: the digits were shown, then the approval ended (expired, answered, denied). + let (client, mut engine) = setup(Reveal::Works); + let mut approval = started(&client, &engine).await; + client.approval_state(&mut approval).await.expect("state"); + client.api().state.lock().expect("lock").ended = true; + assert_eq!( + client.confirm_approval(&mut engine, approval).await.err(), + Some(DeviceError::ApprovalExpired) + ); + assert!(!client.api().state.lock().expect("lock").approved); + + // Deny: nothing is sent to end an approval that is already over. + let (client, engine) = setup(Reveal::Works); + let approval = started(&client, &engine).await; + client.api().state.lock().expect("lock").ended = true; + assert_eq!( + client.deny_approval(approval).await.err(), + Some(DeviceError::ApprovalExpired) + ); + assert_eq!(count(&client, "cancel"), 0); + assert!(!client.api().state.lock().expect("lock").approved); + + // And an approval that was denied once cannot be denied or confirmed again. + let (client, mut engine) = setup(Reveal::Works); + let mut approval = started(&client, &engine).await; + client.approval_state(&mut approval).await.expect("state"); + client.api().state.lock().expect("lock").ended = true; + assert_eq!( + client.approval_state(&mut approval).await.err(), + Some(DeviceError::ApprovalExpired) + ); + assert_eq!( + client.confirm_approval(&mut engine, approval).await.err(), + Some(DeviceError::ApprovalExpired) + ); +} + +#[tokio::test] +async fn a_view_that_does_not_match_what_this_device_started_cancels_the_approval() { + let (client, engine) = setup(Reveal::Works); + let mut approval = started(&client, &engine).await; + // The server shows another commitment than the one this device made: a real mismatch. + client.api().state.lock().expect("lock").commitment = Some(b64(&[9u8; 32])); + assert_eq!( + client.approval_state(&mut approval).await.err(), + Some(DeviceError::ApprovalMismatch) + ); + assert_eq!(count(&client, "reveal"), 0, "nothing was revealed"); + assert_eq!(count(&client, "cancel"), 1, "a real mismatch cancels"); +} diff --git a/crates/orbyn-server/tests/device_client.rs b/crates/orbyn-server/tests/device_client.rs index 07fd4d1..404700e 100644 --- a/crates/orbyn-server/tests/device_client.rs +++ b/crates/orbyn-server/tests/device_client.rs @@ -914,3 +914,48 @@ async fn a_device_whose_name_has_spaces_is_approved_with_matching_digits() { assert_eq!(phone.name, "Phone"); assert_eq!(phone.state, wire::DeviceState::Trusted); } + +#[tokio::test] +#[ignore = "needs DATABASE_URL"] +async fn the_approver_cannot_confirm_or_deny_an_approval_the_new_device_already_ended() { + let server = Server::start().await; + let (a, mut engine_a, b, engine_b) = pending_pair(&server).await; + + // Confirm after the new device cancelled: refused by the server, nothing is approved. + let mut approval = a + .start_approval(&engine_a, &device_id_of(&b)) + .await + .expect("start"); + let joined = b + .join_approval(&engine_b) + .await + .expect("join") + .expect("an approval"); + assert!(matches!( + a.approval_state(&mut approval).await.expect("state"), + ApproverState::Digits { .. } + )); + assert!(a.time_left(&approval).is_some_and(|t| t.as_secs() > 500)); + b.deny_joined(joined).await.expect("deny"); + assert_eq!( + a.confirm_approval(&mut engine_a, approval).await.err(), + Some(DeviceError::ApprovalExpired) + ); + assert_eq!(b.pending_approval().await.expect("current"), None); + + // Deny of an approval that is already over says so. + let approval = a + .start_approval(&engine_a, &device_id_of(&b)) + .await + .expect("start again"); + let joined = b + .join_approval(&engine_b) + .await + .expect("join") + .expect("an approval"); + b.deny_joined(joined).await.expect("deny"); + assert_eq!( + a.deny_approval(approval).await.err(), + Some(DeviceError::ApprovalExpired) + ); +} diff --git a/docs/device-client.md b/docs/device-client.md index 09707d3..b20adb2 100644 --- a/docs/device-client.md +++ b/docs/device-client.md @@ -117,6 +117,32 @@ The client only moves messages between the engine and the server, in order. Ever The label is the device's own signed claim, never proof of which app is running. +### Mapping an approve/deny list onto the client + +An app adapter that shows "devices waiting for approval" builds its requests from `devices()` and drives one approval per request: + +```rust +// A request per device this device does not trust yet (the engine decides what it trusts). +let list = client.devices().await?; +let waiting = list.devices.iter().filter(|d| d.state == DeviceState::Pending); + +let mut approval = client.start_approval(&engine, &device.id).await?; +loop { + match client.approval_state(&mut approval).await? { + ApproverState::Waiting => { /* the new device has not joined: wait, show client.time_left(&approval) */ } + ApproverState::Digits { digits, fingerprint } => { /* show them, ask the person */ break; } + } +} +// "They match": client.confirm_approval(&mut engine, approval).await?; +// "They differ": client.deny_approval(approval).await?; +``` + +- **Expired, answered or given up.** An approval that expired, was cancelled or answered by either device, or was used gives `DeviceError::ApprovalExpired` from `approval_state`, `confirm_approval` and `deny_approval`, and nothing is approved: the server refuses a confirm after the end, and a deny reads the approval first so it sends no second cancel. Show it as "this request ended" and start a new one if the person still wants it. +- **A network failure is not an answer.** The reveal is sent up to three times and, before each repeat, the server is asked whether it already holds it; a failure that outlasts the tries is `DeviceError::Transport` and leaves the approval open, so call `approval_state` again. Only a real mismatch (other keys, another commitment) cancels it. +- **Time left.** `time_left(&approval)` is the server's expiry (from `start_approval`, refreshed by each `approval_state`) minus this device's clock, zero once passed and `None` when the time could not be read. It is a display hint: the server decides. +- **Deny carries no reason yet.** `deny_approval` ends the approval and says nothing more to the new device; a reason would need a wire change. +- `ApproverState`'s `Debug` prints `Digits { .. }`: the digits and the fingerprint are never in a log line. + ### Not here yet The account's protection chain is not signed by the client; the host's first protection setting follows the rules in [architecture.md](architecture.md). From caaab7e2b0cd8dad6baea7acba9c86d23b67ee84 Mon Sep 17 00:00:00 2001 From: Tran Duc Anh Dang Date: Thu, 12 Nov 2026 12:12:12 +1100 Subject: [PATCH 2/2] fix(device-client): ask the server before resending a kept reveal A reveal whose answer was lost, followed by a failed read, left the approver stuck: every later send got 409 and the digits never showed. The server is now asked whether it holds the nonce before a kept reveal is sent again and after a 409, and held counts as sent. --- crates/orbyn-device-client/src/approvals.rs | 45 ++++++++++++------ crates/orbyn-device-client/tests/approvals.rs | 46 +++++++++++++++++++ docs/device-client.md | 2 +- 3 files changed, 79 insertions(+), 14 deletions(-) diff --git a/crates/orbyn-device-client/src/approvals.rs b/crates/orbyn-device-client/src/approvals.rs index 1aaa93d..f10ef65 100644 --- a/crates/orbyn-device-client/src/approvals.rs +++ b/crates/orbyn-device-client/src/approvals.rs @@ -200,8 +200,10 @@ impl DeviceClient { } }, }; + // A reveal kept from an earlier try may have reached the server: ask before sending. + let resend = approval.unsent_reveal.is_some(); approval.unsent_reveal = Some(reveal.clone()); - self.send_reveal(&id, &reveal).await?; + self.send_reveal(&id, &reveal, resend).await?; approval.revealed = true; approval.unsent_reveal = None; } @@ -215,11 +217,30 @@ impl DeviceClient { }) } + /// Whether the server already holds this reveal: its view of the approval shows our nonce. + async fn server_holds(&self, id: &str, reveal: &wire::ApprovalNonce) -> bool { + self.authed(Method::Get, &Self::approval_path(id, None), None) + .await + .ok() + .and_then(|r| parse::(&r.body).ok()) + .is_some_and(|v| v.approver_nonce.as_deref() == Some(&reveal.nonce)) + } + /// Sends the reveal, again if the network fails. A failure never cancels the approval: this - /// device's nonce may have arrived, so before sending again the server is asked whether it - /// holds it. Only an answer from the server that refuses the reveal ends the approval, and the + /// device's nonce may have arrived, and the server takes it once (a second send is 409 + /// `approval_out_of_order`). So the server is asked whether it holds the nonce before a kept + /// reveal is sent again (`resend`), after a network failure, and after that 409; "held" counts + /// as sent. Only an answer from the server that refuses the reveal ends the approval, and the /// server does that itself. - async fn send_reveal(&self, id: &str, reveal: &wire::ApprovalNonce) -> Result<(), DeviceError> { + async fn send_reveal( + &self, + id: &str, + reveal: &wire::ApprovalNonce, + resend: bool, + ) -> Result<(), DeviceError> { + if resend && self.server_holds(id, reveal).await { + return Ok(()); + } let mut last = DeviceError::Transport; for _ in 0..REVEAL_TRIES { let sent = self @@ -231,17 +252,15 @@ impl DeviceClient { .await; match sent { Ok(_) => return Ok(()), - Err(DeviceError::Transport) => { - last = DeviceError::Transport; - let held = self - .authed(Method::Get, &Self::approval_path(id, None), None) - .await - .ok() - .and_then(|r| parse::(&r.body).ok()) - .is_some_and(|v| v.approver_nonce.as_deref() == Some(&reveal.nonce)); - if held { + Err(error @ (DeviceError::Transport | DeviceError::ApprovalNotReady)) => { + if self.server_holds(id, reveal).await { return Ok(()); } + if error == DeviceError::ApprovalNotReady { + // Not held and still out of order: it is not ours to retry. + return Err(error); + } + last = error; } Err(other) => return Err(other), } diff --git a/crates/orbyn-device-client/tests/approvals.rs b/crates/orbyn-device-client/tests/approvals.rs index 56672e3..d7c2507 100644 --- a/crates/orbyn-device-client/tests/approvals.rs +++ b/crates/orbyn-device-client/tests/approvals.rs @@ -42,6 +42,8 @@ struct State { ended: bool, expires_at: String, approved: bool, + /// Reads of the approval that fail with the network once the server holds our nonce. + get_fails: usize, /// What the server shows as the commitment, when it lies about it. commitment: Option, log: Vec, @@ -102,6 +104,10 @@ impl DeviceApi for Script { if s.ended { return gone(); } + if s.approver_nonce.is_some() && s.get_fails > 0 { + s.get_fails -= 1; + return Err(DeviceError::Transport); + } let create = s.create.clone().expect("created first"); answer( 200, @@ -120,6 +126,11 @@ impl DeviceApi for Script { }), ) } + // The server takes the nonce once, as the real one does. + (Method::Post, _) if step == "reveal" && s.approver_nonce.is_some() => answer( + 409, + json!({"code": "approval_out_of_order", "message": "x"}), + ), (Method::Post, _) if step == "reveal" => match s.reveal { Reveal::NetworkFails(n) if n > 0 => { s.reveal = Reveal::NetworkFails(n - 1); @@ -193,6 +204,7 @@ fn setup(reveal: Reveal) -> (Client, Engine) { ended: false, expires_at: EXPIRES.to_owned(), approved: false, + get_fails: 0, commitment: None, log: Vec::new(), }), @@ -371,3 +383,37 @@ async fn a_view_that_does_not_match_what_this_device_started_cancels_the_approva assert_eq!(count(&client, "reveal"), 0, "nothing was revealed"); assert_eq!(count(&client, "cancel"), 1, "a real mismatch cancels"); } + +#[tokio::test] +async fn a_lost_reveal_answer_and_a_failed_read_do_not_leave_the_approver_stuck() { + let (client, engine) = setup(Reveal::AnswerLostOnce); + let mut approval = started(&client, &engine).await; + // The reveal reaches the server and the answer is lost; the two reads that ask whether the + // server holds the nonce fail as well, and the second send is answered 409. + client.api().state.lock().expect("lock").get_fails = 2; + assert_eq!( + client.approval_state(&mut approval).await.err(), + Some(DeviceError::ApprovalNotReady) + ); + assert_eq!(count(&client, "cancel"), 0); + // The next call asks first, finds the nonce held and shows the digits: it does not send again. + let state = client + .approval_state(&mut approval) + .await + .expect("recovers"); + assert!(matches!(state, ApproverState::Digits { .. })); + assert_eq!(count(&client, "reveal"), 2, "the third send never happened"); + assert_eq!(count(&client, "cancel"), 0); +} + +#[tokio::test] +async fn a_409_after_a_lost_answer_is_settled_by_asking_the_server() { + let (client, engine) = setup(Reveal::AnswerLostOnce); + let mut approval = started(&client, &engine).await; + // One failed read: the second send is answered 409, and the read that follows finds the nonce. + client.api().state.lock().expect("lock").get_fails = 1; + let state = client.approval_state(&mut approval).await.expect("state"); + assert!(matches!(state, ApproverState::Digits { .. })); + assert_eq!(count(&client, "reveal"), 2); + assert_eq!(count(&client, "cancel"), 0); +} diff --git a/docs/device-client.md b/docs/device-client.md index b20adb2..7d3b087 100644 --- a/docs/device-client.md +++ b/docs/device-client.md @@ -138,7 +138,7 @@ loop { ``` - **Expired, answered or given up.** An approval that expired, was cancelled or answered by either device, or was used gives `DeviceError::ApprovalExpired` from `approval_state`, `confirm_approval` and `deny_approval`, and nothing is approved: the server refuses a confirm after the end, and a deny reads the approval first so it sends no second cancel. Show it as "this request ended" and start a new one if the person still wants it. -- **A network failure is not an answer.** The reveal is sent up to three times and, before each repeat, the server is asked whether it already holds it; a failure that outlasts the tries is `DeviceError::Transport` and leaves the approval open, so call `approval_state` again. Only a real mismatch (other keys, another commitment) cancels it. +- **A network failure is not an answer.** The reveal is sent up to three times. The server takes it once (a second send is a 409), so it is asked whether it already holds it before a kept reveal is sent again, after a network failure and after that 409, and "held" counts as sent; a failure that outlasts the tries is `DeviceError::Transport` and leaves the approval open, so call `approval_state` again. Only a real mismatch (other keys, another commitment) cancels it. - **Time left.** `time_left(&approval)` is the server's expiry (from `start_approval`, refreshed by each `approval_state`) minus this device's clock, zero once passed and `None` when the time could not be read. It is a display hint: the server decides. - **Deny carries no reason yet.** `deny_approval` ends the approval and says nothing more to the new device; a reason would need a wire change. - `ApproverState`'s `Debug` prints `Digits { .. }`: the digits and the fingerprint are never in a log line.