fix(qbzd): mask secret values in import summary and distinguish reload refusal
cc
This commit is contained in:
4 files changed
+276
-27
No files matched your search
@@ -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<String, Value>, current: &AudioSettings) -> String {
|
||||
let bundle_backend: Option<AudioBackendType> = 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"));
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
@@ -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<String> {
|
||||
if let Ok(t) = std::env::var("QBZD_TOKEN") {
|
||||
if !t.is_empty() {
|
||||
@@ -656,9 +664,10 @@ pub fn export(roots: &ProfileRoots, file: Option<String>, 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<String>, 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");
|
||||
|
||||
+45
-11
@@ -202,18 +202,40 @@ pub fn logout(roots: &ProfileRoots) -> Result<bool, LoginError> {
|
||||
Ok(nudge_reload(&host, None))
|
||||
}
|
||||
|
||||
/// 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 {
|
||||
if !http_request_2xx(host, "GET", "/api/ping", token) {
|
||||
return false;
|
||||
/// 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,
|
||||
}
|
||||
http_request_2xx(host, "POST", "/api/settings/reload", token)
|
||||
|
||||
/// Best-effort `GET /api/ping` → `POST /api/settings/reload` against a local
|
||||
/// 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 NudgeOutcome::DaemonDown;
|
||||
}
|
||||
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();
|
||||
|
||||
Reference in new issue
Block a user