diff --git a/crates/qbz-app/src/settings/bundle.rs b/crates/qbz-app/src/settings/bundle.rs index 62dc5b1a5..d461e44d8 100644 --- a/crates/qbz-app/src/settings/bundle.rs +++ b/crates/qbz-app/src/settings/bundle.rs @@ -115,6 +115,9 @@ pub struct PlanLine { #[derive(Debug, Clone)] pub struct DevicePick { pub wanted: String, + /// The backend the options were enumerated for — the §5.4 prompt names it + /// ("Available on Alsa:"). + pub backend: String, pub options: Vec<(String, String)>, } @@ -209,6 +212,37 @@ impl Bundle { serde_json::to_string_pretty(self).map_err(|e| BundleError::Io(e.to_string())) } + /// True when the document actually CARRIES a secret value: an `auth` token, + /// or a non-blank scrobbler secret. The export-side §3 warning keys on this + /// — not on the auth domain alone (a bundle whose only secrets are scrobbler + /// tokens still needs the warning). + pub fn contains_secrets(&self) -> bool { + let auth_token = self + .domains + .get("auth") + .and_then(Value::as_object) + .and_then(|a| a.get("user_auth_token")) + .and_then(Value::as_str) + .map(|t| !t.is_empty()) + .unwrap_or(false); + if auth_token { + return true; + } + self.domains + .get("integrations") + .and_then(|i| i.get("scrobblers")) + .and_then(Value::as_object) + .map(|s| { + ["lastfm_session_key", "listenbrainz_token"].iter().any(|k| { + s.get(*k) + .and_then(Value::as_str) + .map(|v| !v.is_empty()) + .unwrap_or(false) + }) + }) + .unwrap_or(false) + } + /// Parse a bundle from JSON text (import step 1). The version gate (step 2) /// runs in [`plan`]; here we only require a JSON object with an integer /// `schema_version`, so a malformed version is caught early. @@ -555,6 +589,24 @@ fn applied_line(plan: &mut ImportPlan, key: &str, value: &Value, why: &str) { plan.writes.push((key.to_string(), value.clone())); } +/// SECRET-class applied line (§5.4): the VALUE COLUMN IS MASKED — the real +/// value goes only into the write list, never into the rendered summary +/// (terminal scrollback and CI logs are not a place for bearer tokens). +/// Non-empty → `(secret, applied)`; empty → `(empty)`, matching the §5.4 example. +fn applied_secret_line(plan: &mut ImportPlan, key: &str, value: &Value) { + let masked = match value.as_str() { + Some(s) if !s.is_empty() => "(secret, applied)", + _ => "(empty)", + }; + plan.applied.push(PlanLine { + key: key.to_string(), + old: None, + new: masked.to_string(), + why: String::new(), + }); + plan.writes.push((key.to_string(), value.clone())); +} + fn adapted_line(plan: &mut ImportPlan, key: &str, old: &Value, new: &Value, why: &str) { plan.adapted.push(PlanLine { key: key.to_string(), @@ -761,6 +813,7 @@ fn plan_audio_machine( if !opts.non_tty { plan.device_pick = Some(DevicePick { wanted: bundle_device.clone().unwrap_or_default(), + backend: pick_backend_name(map, current), options: live.devices.clone(), }); } @@ -897,7 +950,7 @@ fn plan_audio_machine( "audio.dsd_mode", v, &Value::String("convert".into()), - "pass --trust-dsd to keep DoP/native", + "pass --trust-dsd to keep DoP", ); } else { applied_line(plan, "audio.dsd_mode", v, ""); @@ -905,6 +958,20 @@ fn plan_audio_machine( } } +/// The backend the device options belong to: the bundle's backend when present, +/// else the target's current, else system default (for the §5.4 prompt line). +fn pick_backend_name(map: &Map, current: &AudioSettings) -> String { + let bundle_backend: Option = map + .get("backend_type") + .and_then(|v| serde_json::from_value(v.clone()).ok()); + backend_name( + bundle_backend + .or(current.backend_type) + .unwrap_or(AudioBackendType::SystemDefault), + ) + .to_string() +} + fn backend_name(b: AudioBackendType) -> &'static str { match b { AudioBackendType::PipeWire => "PipeWire", @@ -1027,7 +1094,7 @@ fn plan_integrations(value: &Value, opts: &ImportOptions, uid_will_exist: bool, applied_line(plan, &full, v, ""); } else if SCROBBLER_SECRET.contains(&k.as_str()) { if opts.include_auth { - applied_line(plan, &full, v, "secret, applied"); + applied_secret_line(plan, &full, v); } else { plan.skipped .push(skip_line(&full, "secrets require --include-auth")); diff --git a/crates/qbz-app/src/settings/bundle/tests.rs b/crates/qbz-app/src/settings/bundle/tests.rs index 0250a2c0a..f5b76574d 100644 --- a/crates/qbz-app/src/settings/bundle/tests.rs +++ b/crates/qbz-app/src/settings/bundle/tests.rs @@ -503,3 +503,74 @@ fn parse_rejects_missing_version() { let err = Bundle::parse(r#"{ "audio": {} }"#).unwrap_err(); assert!(matches!(err, BundleError::VersionMalformed)); } + +#[test] +fn secret_values_never_render_in_summary() { + // §5.4: SECRET-class values are MASKED in the summary lines — the raw value + // rides only the write list (terminal scrollback / CI logs must never see it). + let p = scratch("mask"); + std::fs::create_dir_all(&p.data_root).unwrap(); + write_last_user_id(&p.data_root, 1).unwrap(); + let bundle = bundle_with(json!({ + "integrations": { "scrobblers": { + "lastfm_session_key": "d580REALSECRET", + "listenbrainz_token": "" + } } + })); + let opts = ImportOptions { include_auth: true, ..Default::default() }; + + let plan = plan(&bundle, &p, &opts, &live()).expect("plan"); + + let key_line = find(&plan.applied, "integrations.scrobblers.lastfm_session_key").unwrap(); + assert_eq!(key_line.new, "(secret, applied)"); + let empty_line = find(&plan.applied, "integrations.scrobblers.listenbrainz_token").unwrap(); + assert_eq!(empty_line.new, "(empty)"); + + // No rendered bucket line anywhere carries the raw secret. + let rendered = format!("{:?} {:?} {:?}", plan.applied, plan.adapted, plan.skipped); + assert!(!rendered.contains("d580REALSECRET"), "secret leaked into summary: {rendered}"); + + // The write list still carries the real value so apply works. + assert_eq!( + write_of(&plan, "integrations.scrobblers.lastfm_session_key"), + Some(&json!("d580REALSECRET")) + ); + cleanup(&p); +} + +#[test] +fn contains_secrets_keys_on_actual_secret_values() { + // §3 warning trigger: any secret VALUE present (auth token OR a non-blank + // scrobbler secret) — not the auth domain alone. + let none = bundle_with(json!({ "audio": { "gapless_enabled": true } })); + assert!(!none.contains_secrets()); + + let blank = bundle_with(json!({ + "integrations": { "scrobblers": { "lastfm_session_key": "", "listenbrainz_token": "" } } + })); + assert!(!blank.contains_secrets(), "blank secrets are not secrets"); + + let scrob = bundle_with(json!({ + "integrations": { "scrobblers": { "lastfm_session_key": "sk-live" } } + })); + assert!(scrob.contains_secrets()); + + let auth = bundle_with(json!({ "auth": { "user_auth_token": "tok" } })); + assert!(auth.contains_secrets()); +} + +#[test] +fn device_pick_names_the_backend() { + // §5.4 prompt: "Available on Alsa:" — the pick carries the backend name. + let p = scratch("pick-backend"); + let bundle = bundle_with(json!({ + "audio": { "output_device": "hw:9,9", "backend_type": "Alsa" } + })); + + let plan = plan(&bundle, &p, &ImportOptions::default(), &live()).expect("plan"); + + let pick = plan.device_pick.expect("TTY plan must request a pick"); + assert_eq!(pick.backend, "Alsa"); + assert_eq!(pick.wanted, "hw:9,9"); + cleanup(&p); +} diff --git a/crates/qbzd/src/cli/settings.rs b/crates/qbzd/src/cli/settings.rs index 615b0c725..2a6b5a5da 100644 --- a/crates/qbzd/src/cli/settings.rs +++ b/crates/qbzd/src/cli/settings.rs @@ -432,6 +432,14 @@ fn nudge(roots: &ProfileRoots) -> bool { crate::login::nudge_reload(&host, token.as_deref()) } +/// Three-state variant of [`nudge`] for `settings import`, which must +/// distinguish daemon-down from reload-refused (04 §5.3 step 7). +fn nudge_outcome(roots: &ProfileRoots) -> crate::login::NudgeOutcome { + let host = crate::login::nudge_host(roots); + let token = local_token(roots); + crate::login::nudge_reload_outcome(&host, token.as_deref()) +} + fn local_token(roots: &ProfileRoots) -> Option { if let Ok(t) = std::env::var("QBZD_TOKEN") { if !t.is_empty() { @@ -656,9 +664,10 @@ pub fn export(roots: &ProfileRoots, file: Option, from: &str, include_au return 1; } - // The §3 warning prints only when secrets actually made it into the bundle - // (--include-auth with a decryptable token present). - if bundle.domains.contains_key("auth") { + // The §3 warning prints whenever ANY secret actually made it into the file: + // the auth token OR a non-blank scrobbler secret (--include-auth exports + // scrobbler tokens even when the Qobuz token itself is absent). + if bundle.contains_secrets() { println!("{}", crate::cli::copy::bundle_secret_warning(&path)); } else { println!("{}", crate::cli::copy::bundle_export_success(&path)); @@ -795,16 +804,52 @@ pub async fn import( return 1; } - // Step 7: reload-nudge a running daemon. - let reloaded = nudge(roots); - let reload_line = if reloaded { - "daemon reloaded (was running)" - } else { - "daemon not running (changes apply on next start)" - }; + // Step 7: reload-nudge a running daemon. Three states (04 §5.3 step 7): + // reloaded / not running (fine) / up-but-refused (exit 1, restart hint). + let outcome = nudge_outcome(roots); + let (done_line, stderr_msg, exit) = + reload_disposition(outcome, plan.routing_critical_changed); - print_buckets(&plan, &bundle, auth_note.as_deref(), Some(reload_line)); - 0 + print_buckets(&plan, &bundle, auth_note.as_deref(), Some(&done_line)); + if let Some(msg) = stderr_msg { + eprintln!("\n{msg}"); + } + exit +} + +/// Pure mapping of the nudge outcome (+ whether a routing-critical field +/// changed) to the `done:` reload phrase, an optional stderr error, and the +/// exit code. Split from IO so the §5.3-step-7 contract is unit-testable. +fn reload_disposition( + outcome: crate::login::NudgeOutcome, + routing_critical: bool, +) -> (String, Option, i32) { + use crate::login::NudgeOutcome::*; + match outcome { + Reloaded => { + // §5.3 step 7 honesty rule: a routing-critical change re-inits the + // output device on the spot — say so instead of hiding it. + let line = if routing_critical { + "daemon reloaded (was running; output device reinitialized — an in-flight track may gap)" + } else { + "daemon reloaded (was running)" + }; + (line.to_string(), None, 0) + } + DaemonDown => ( + "daemon not running (changes apply on next start)".to_string(), + None, + 0, + ), + ReloadRefused => ( + "daemon answered ping but refused the reload".to_string(), + Some( + "error: settings saved but the daemon did not reload — restart it: systemctl --user restart qbzd" + .to_string(), + ), + 1, + ), + } } /// Enumerate the local audio system for [`bundle::plan`]: the available backends @@ -833,8 +878,8 @@ fn build_live_system(bundle: &Bundle) -> LiveSystem { /// always "system default"; an unparseable answer falls to system default. fn prompt_device(pick: &bundle::DevicePick) -> DeviceChoice { println!( - "audio device \"{}\" not found on this machine. Available:", - pick.wanted + "audio device \"{}\" not found on this machine. Available on {}:", + pick.wanted, pick.backend ); for (i, (id, label)) in pick.options.iter().enumerate() { println!(" [{}] {} {}", i + 1, id, label); @@ -1127,6 +1172,38 @@ mod tests { cleanup(&roots); } + #[test] + fn reload_disposition_maps_the_three_outcomes() { + use crate::login::NudgeOutcome::*; + + let (line, err, code) = reload_disposition(Reloaded, false); + assert_eq!(line, "daemon reloaded (was running)"); + assert!(err.is_none()); + assert_eq!(code, 0); + + // §5.3 step 7 honesty rule: routing-critical + reloaded names the gap. + let (line, err, code) = reload_disposition(Reloaded, true); + assert!(line.contains("output device reinitialized"), "{line}"); + assert!(line.contains("gap"), "{line}"); + assert!(err.is_none()); + assert_eq!(code, 0); + + // Daemon simply not running is NOT an error, routing-critical or not. + let (line, err, code) = reload_disposition(DaemonDown, true); + assert_eq!(line, "daemon not running (changes apply on next start)"); + assert!(err.is_none()); + assert_eq!(code, 0); + + // Up-but-refused → exit 1 with the verbatim restart hint. + let (_, err, code) = reload_disposition(ReloadRefused, false); + let msg = err.expect("refused must carry the stderr copy"); + assert_eq!( + msg, + "error: settings saved but the daemon did not reload — restart it: systemctl --user restart qbzd" + ); + assert_eq!(code, 1); + } + #[test] fn present_keys_reports_nested_and_top_level_keys() { let roots = scratch_roots("config-present"); diff --git a/crates/qbzd/src/login.rs b/crates/qbzd/src/login.rs index 90167b4bd..0c2087e59 100644 --- a/crates/qbzd/src/login.rs +++ b/crates/qbzd/src/login.rs @@ -202,18 +202,40 @@ pub fn logout(roots: &ProfileRoots) -> Result { Ok(nudge_reload(&host, None)) } +/// Three-state outcome of the ping-then-reload nudge. 04-settings-portability.md +/// §5.3 step 7 needs "daemon simply not running" (not an error) distinguished +/// from "daemon up but the reload was refused/500" (exit 1 with the restart +/// hint) — a single bool conflates them. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum NudgeOutcome { + /// Ping answered and the reload returned 2xx. + Reloaded, + /// Ping did not answer — no daemon to nudge (never an error). + DaemonDown, + /// Ping answered but the reload did not return 2xx. + ReloadRefused, +} + /// Best-effort `GET /api/ping` → `POST /api/settings/reload` against a local -/// daemon (the reload route lands in T11). Any failure — daemon down, route not -/// yet present, timeout — returns `false`, which every caller treats as "the -/// daemon will pick the credential change up on its next start" (login/logout -/// are specified to work daemon-down, 02 §2.2). `token` carries the opt-in -/// `[server] token` as `Authorization: Bearer` when present; T5 callers pass -/// `None`. -pub fn nudge_reload(host: &str, token: Option<&str>) -> bool { +/// daemon. `token` carries the opt-in `[server] token` as +/// `Authorization: Bearer` when present; T5 callers pass `None`. +pub fn nudge_reload_outcome(host: &str, token: Option<&str>) -> NudgeOutcome { if !http_request_2xx(host, "GET", "/api/ping", token) { - return false; + return NudgeOutcome::DaemonDown; } - http_request_2xx(host, "POST", "/api/settings/reload", token) + if http_request_2xx(host, "POST", "/api/settings/reload", token) { + NudgeOutcome::Reloaded + } else { + NudgeOutcome::ReloadRefused + } +} + +/// Boolean skin over [`nudge_reload_outcome`] for the callers that only need +/// "did a running daemon acknowledge?" (login/logout/`settings set` — they are +/// specified to work daemon-down, 02 §2.2, so any non-reload is just "the +/// daemon picks it up on next start"). +pub fn nudge_reload(host: &str, token: Option<&str>) -> bool { + nudge_reload_outcome(host, token) == NudgeOutcome::Reloaded } // ============================ pure, unit-tested ============================ @@ -641,6 +663,18 @@ mod tests { assert!(!nudge_reload(&format!("127.0.0.1:{port}"), None)); } + #[test] + fn nudge_outcome_is_daemon_down_when_nothing_listens() { + // The three-state outcome: no listener → DaemonDown (never ReloadRefused). + let l = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = l.local_addr().unwrap().port(); + drop(l); + assert_eq!( + nudge_reload_outcome(&format!("127.0.0.1:{port}"), None), + NudgeOutcome::DaemonDown + ); + } + #[test] fn login_timeout_error_renders_the_verbatim_copy_with_the_port() { let rendered = LoginError::Timeout(39114).to_string();