From bd7f09095f8c25928e37fcb05e385e0b932b2e7c Mon Sep 17 00:00:00 2001 From: DSH Agent Date: Sun, 20 Sep 2026 04:08:10 -0400 Subject: [PATCH] v0.1.4: name the failure accurately, and add DA_VAULT_RESET MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No change to how the vault is written or recovered. This fixes what the service says when recovery is not possible, and adds a way out that is not "edit files inside a container that will not start". The refusal message was wrong in the case that actually matters. It said the backup "could not be used either" whether the backup was unparseable, empty, or simply absent — and absent is the common one, because a vault that has not completed a single write since v0.1.3 introduced backups has no .bak at all. That wording reads as though a file exists, and sends the operator hunting for it. The message now names which of the three it is, and lists any vault.json.corrupt-* files left by earlier recoveries. DA_VAULT_RESET=1 boots an unreadable vault as empty. It is checked only on the failure path, so it cannot affect a vault that loads, and it quarantines the unreadable file *before* writing the fresh one — the first draft of this logged that the file was preserved while persist would in fact have overwritten it. Refusing by default is still right: booting empty is indistinguishable from a vault emptied on purpose. But a refusal with no supported way forward is not a safety feature, it is a dead end. 149 tests. --- Cargo.lock | 12 +- Cargo.toml | 2 +- README.md | 29 +++- crates/core/src/vault.rs | 279 ++++++++++++++++++++++++++++++++++++--- docs/releases/v0.1.4.md | 52 ++++++++ 5 files changed, 339 insertions(+), 35 deletions(-) create mode 100644 docs/releases/v0.1.4.md diff --git a/Cargo.lock b/Cargo.lock index a76b215..a06a5e7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -436,7 +436,7 @@ dependencies = [ [[package]] name = "da-core" -version = "0.1.3" +version = "0.1.4" dependencies = [ "argon2", "base32", @@ -461,7 +461,7 @@ dependencies = [ [[package]] name = "da-discord" -version = "0.1.3" +version = "0.1.4" dependencies = [ "axum", "da-core", @@ -478,7 +478,7 @@ dependencies = [ [[package]] name = "da-http-api" -version = "0.1.3" +version = "0.1.4" dependencies = [ "axum", "base64 0.22.1", @@ -500,7 +500,7 @@ dependencies = [ [[package]] name = "da-render" -version = "0.1.3" +version = "0.1.4" dependencies = [ "image", "resvg", @@ -512,7 +512,7 @@ dependencies = [ [[package]] name = "da-webui" -version = "0.1.3" +version = "0.1.4" [[package]] name = "data-url" @@ -542,7 +542,7 @@ dependencies = [ [[package]] name = "discord-authenticator" -version = "0.1.3" +version = "0.1.4" dependencies = [ "anyhow", "axum", diff --git a/Cargo.toml b/Cargo.toml index c0267f5..b82c47e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,7 +10,7 @@ members = [ ] [workspace.package] -version = "0.1.3" +version = "0.1.4" edition = "2021" rust-version = "1.90" license = "MIT OR Apache-2.0" diff --git a/README.md b/README.md index 64875a6..72a64c2 100644 --- a/README.md +++ b/README.md @@ -392,19 +392,36 @@ accounts are loaded from it, the damaged file is moved to `vault.json.corrupt-`, and a fresh `vault.json` is written. You will see one `ERROR` line saying so, then normal startup. -If it still refuses, both files are unreadable and there is nothing to recover -from automatically. Get the file out of the way and restart: +If it still refuses, the message names exactly which situation you are in, +because they need different responses: + +- **"there is no backup at …"** — no `.bak` was ever created, which means this + vault has not completed a single write since backups were introduced. Nothing + on disk is recoverable; the unreadable file is all there is. +- **"exists but could not be parsed either"** — both copies are gone. +- **"is empty as well"** — both are zero bytes. + +Either way, the service will not boot with an empty vault on its own, because +that looks identical to a vault you emptied on purpose. Two ways forward: ```bash +# 1. by hand cd /mnt/user/appdata/discord-authenticator mv vault.json "vault.json.broken-$(date +%s)" docker restart Discord-Authenticator ``` -The service will start with an empty vault, so re-import your accounts from -your own backups (`otpauth://` links or a Google Authenticator export). Keep -`vault.key` — without it, nothing in the old file can be decrypted even if the -bytes are recovered later. +Or set `DA_VAULT_RESET=1` in the Unraid template (Docker → the container → Add +another Path, Variable, Port or Label → Variable, Key `DA_VAULT_RESET`, Value +`1`) and restart. The service boots empty and moves the unreadable file to +`vault.json.corrupt-` before writing a fresh vault, so nothing is +overwritten. **Remove the variable afterwards** — it only has an effect on the +failure path, but leaving it set means the next genuinely-broken vault boots +empty instead of telling you about it. + +Then re-import your accounts from your own backups (`otpauth://` links or a +Google Authenticator export). Keep `vault.key` — without it, nothing in an old +vault file can be decrypted even if the bytes turn up later. **If you hit this, upgrading matters more than the workaround.** The cause was a rename being journalled ahead of the data it published, which is fixed by diff --git a/crates/core/src/vault.rs b/crates/core/src/vault.rs index 4b1989f..42fbe17 100644 --- a/crates/core/src/vault.rs +++ b/crates/core/src/vault.rs @@ -251,6 +251,8 @@ impl Vault { pub fn load(work_dir: &Path, key: VaultKey) -> Result { let path = work_dir.join("vault.json"); let backup_path = backup_of(&path); + // `load_accounts` decides on DA_VAULT_RESET itself, because that is + // where the description of what went wrong is built. let (accounts, recovered) = load_accounts(&path, &backup_path)?; tracing::info!(target: "da::vault", count = accounts.len(), "vault loaded"); @@ -790,6 +792,34 @@ impl Vault { } } +/// Has the operator explicitly asked to start with an empty vault? +/// +/// Set `DA_VAULT_RESET=1` to boot an unreadable vault as empty instead of +/// refusing. Checked only on the failure path, so it has no effect on a vault +/// that loads normally. +fn reset_requested() -> bool { + std::env::var("DA_VAULT_RESET") + .map(|v| looks_like_reset(&v)) + .unwrap_or(false) +} + +/// Split out so the accepted spellings can be tested without touching the +/// process environment. +fn looks_like_reset(value: &str) -> bool { + let v = value.trim(); + !v.is_empty() && v != "0" && !v.eq_ignore_ascii_case("false") +} + +/// Serializes the tests that write process environment variables. Rust runs +/// tests in parallel threads within one process, so an env var set by one test +/// is visible to every other test running at that moment. +#[cfg(test)] +fn env_guard() -> std::sync::MutexGuard<'static, ()> { + static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + LOCK.lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) +} + /// `vault.json` → `vault.json.bak`. fn backup_of(path: &Path) -> PathBuf { let mut name = path @@ -806,8 +836,19 @@ fn backup_of(path: &Path) -> PathBuf { /// between "won't parse" and "unrecoverable", and the backup lives in it: /// whenever one exists and does parse, the honest move is to use it and say so /// loudly, keeping the damaged original for inspection. +/// +/// When *nothing* is recoverable the refusal has to say why precisely, because +/// the two cases need opposite responses from the operator. A backup that is +/// present and unreadable means both copies are gone and the accounts have to +/// be re-added; a backup that was never created (a container whose first write +/// never completed, so this feature has not run once) means the same file is +/// fine and there is simply nothing on disk to recover. The old wording said +/// "the backup could not be used either" for both, which sent the operator +/// looking for a file that did not exist. fn load_accounts(path: &Path, backup_path: &Path) -> Result<(Vec, bool)> { let text = match std::fs::read_to_string(path) { + // A missing vault is a first run, not a failure: there is nothing to + // recover and the service creates one on its first write. Ok(text) => text, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok((Vec::new(), false)), Err(e) => return Err(e.into()), @@ -815,36 +856,116 @@ fn load_accounts(path: &Path, backup_path: &Path) -> Result<(Vec, bool) match serde_json::from_str::(&text) { Ok(file) => Ok((file.accounts, false)), - Err(primary_err) => match read_backup(backup_path) { - Some(accounts) => { - let quarantined = quarantine(path); - tracing::error!( + Err(primary_err) => { + match read_backup(backup_path) { + Some(accounts) => { + let quarantined = quarantine(path); + tracing::error!( + target: "da::vault", + path = %path.display(), + backup = %backup_path.display(), + recovered = accounts.len(), + quarantined = %quarantined.as_deref().map_or_else( + || "could not move aside".to_string(), + |p| p.display().to_string() + ), + "vault.json could not be parsed ({primary_err}); recovered the accounts \ + from the backup. The damaged file is kept alongside for inspection — \ + compare it against the backup if anything you added recently is missing." + ); + return Ok((accounts, true)); + } + None => {} + } + let damage = describe_damage(path, backup_path, &primary_err.to_string()); + + // The refusal is the right default, but it must not be a dead end: + // an operator who has accepted that the accounts are gone needs a + // way forward that does not involve editing files inside a + // container that will not start. The variable has to be set + // deliberately, so this can never look like the silent-empty + // fallback the refusal exists to prevent. + if reset_requested() { + // Move the unreadable file aside *before* the empty vault is + // written, or the caller's `persist` would overwrite it and + // then take the backup from the replacement — destroying the + // only copy of whatever might still have been salvageable. + let kept = quarantine(path); + tracing::warn!( target: "da::vault", - path = %path.display(), - backup = %backup_path.display(), - recovered = accounts.len(), - quarantined = %quarantined.as_deref().map_or_else( - || "could not move aside".to_string(), + preserved = %kept.as_deref().map_or_else( + || "NOTHING — it could not be moved aside and will be overwritten" + .to_string(), |p| p.display().to_string() ), - "vault.json could not be parsed ({primary_err}); recovered the accounts \ - from the backup. The damaged file is kept alongside for inspection — \ - compare it against the backup if anything you added recently is missing." + "DA_VAULT_RESET is set, so starting with an empty vault. {damage}" ); - Ok((accounts, true)) + return Ok((Vec::new(), true)); } - None => Err(Error::invalid(format!( - "{} exists but could not be parsed ({primary_err}), and the backup at {} \ - could not be used either. Refusing to start with an empty vault — move the \ - file aside to start fresh.", - path.display(), - backup_path.display() - ))), - }, + + Err(Error::invalid(format!( + "{damage} Refusing to start with an empty vault, because that would look \ + exactly like a vault you had emptied on purpose. If those accounts are gone, \ + either move the files aside to start fresh, or set DA_VAULT_RESET=1 to do the \ + same thing from the Unraid template — either way a copy of the unreadable file \ + is kept." + ))) + } } } -/// Parse the backup, or `None` if it is missing or not parseable as a vault. +/// Describe exactly what is on disk and why nothing could be loaded. +/// +/// The wording has to distinguish the cases, because they need opposite +/// responses. A backup that is present and unreadable means both copies are +/// gone and the accounts must be re-added. A backup that was never created — +/// a container that has not completed a single write since backups were +/// introduced — means there is simply nothing on disk to recover. The previous +/// message said "the backup could not be used either" for both, which sent an +/// operator hunting for a file that did not exist. +/// +/// Returns only the description. The decision to refuse, and the advice that +/// goes with it, belongs to the caller: a caller that has been told to reset is +/// not refusing anything, and must not say that it is. +fn describe_damage(path: &Path, backup_path: &Path, primary_err: &str) -> String { + let backup_note = match std::fs::metadata(backup_path) { + Ok(m) if m.len() == 0 => format!( + "the backup at {} is empty as well, so there is nothing to recover", + backup_path.display() + ), + Ok(_) => format!( + "the backup at {} exists but could not be parsed either", + backup_path.display() + ), + Err(_) => format!( + "there is no backup at {} — this vault has not been written since \ + backups were introduced, so none was ever created", + backup_path.display() + ), + }; + + // A quarantine file means a previous boot already recovered this vault and + // moved a damaged copy aside. That copy is the only other candidate left. + let quarantined: Vec = std::fs::read_dir(path.parent().unwrap_or(Path::new("."))) + .into_iter() + .flatten() + .flatten() + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.starts_with("vault.json.corrupt-")) + .collect(); + let quarantine_note = if quarantined.is_empty() { + String::new() + } else { + format!( + " Files kept from earlier recoveries that may be worth inspecting: {}.", + quarantined.join(", ") + ) + }; + + format!("vault.json could not be parsed ({primary_err}), and {backup_note}.{quarantine_note}") +} + +/// Parse the backup, or `None` if it is absent or not parseable as a vault. fn read_backup(backup_path: &Path) -> Option> { let text = std::fs::read_to_string(backup_path).ok()?; serde_json::from_str::(&text) @@ -1318,6 +1439,120 @@ mod tests { ); } + /// The refusal has to say which of the two situations the operator is in: + /// both copies gone, or a backup that was never created. The old wording + /// said "the backup could not be used either" for both, which sent someone + /// hunting for a file that did not exist. + #[test] + fn the_refusal_distinguishes_a_missing_backup_from_a_broken_one() { + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + + let missing = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()) + .unwrap_err() + .to_string(); + // No .bak at all: this is the state a container is in when it has not + // completed a single write since backups were introduced. + assert!( + missing.contains("no backup at") && missing.contains("has not been written since"), + "a missing backup must be named as missing: {missing}" + ); + + std::fs::write(dir.path().join("vault.json.bak"), b"garbage").unwrap(); + let broken = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()) + .unwrap_err() + .to_string(); + assert!( + broken.contains("exists but could not be parsed"), + "a backup that is present but unreadable must say so: {broken}" + ); + } + + #[test] + fn an_empty_backup_is_reported_as_empty_not_as_broken() { + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + std::fs::write(dir.path().join("vault.json.bak"), b"").unwrap(); + + let msg = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()) + .unwrap_err() + .to_string(); + assert!( + msg.contains("is empty as well"), + "zero bytes is not the same as unparseable: {msg}" + ); + } + + #[test] + fn the_refusal_points_at_anything_kept_by_an_earlier_recovery() { + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + std::fs::write( + dir.path().join("vault.json.corrupt-1700000000"), + b"an earlier damaged copy", + ) + .unwrap(); + + let msg = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()) + .unwrap_err() + .to_string(); + assert!( + msg.contains("vault.json.corrupt-1700000000"), + "a copy from an earlier recovery is the only other thing worth \ + looking at, so it should be named: {msg}" + ); + } + + #[test] + fn reset_is_only_requested_by_a_deliberate_value() { + assert!(looks_like_reset("1")); + assert!(looks_like_reset("yes")); + assert!(looks_like_reset(" true ")); + // Explicitly off, and the ways an unset variable shows up. + assert!(!looks_like_reset("0")); + assert!(!looks_like_reset("false")); + assert!(!looks_like_reset("FALSE")); + assert!(!looks_like_reset("")); + assert!(!looks_like_reset(" ")); + } + + /// The escape hatch itself. Serialized against the greeting test because + /// both write process environment variables, which are shared by every + /// test in the binary. + #[test] + fn da_vault_reset_boots_empty_and_preserves_the_unreadable_file() { + let _guard = env_guard(); + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + std::fs::write(dir.path().join("vault.json.bak"), b"").unwrap(); + + std::env::set_var("DA_VAULT_RESET", "1"); + let loaded = Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()); + std::env::remove_var("DA_VAULT_RESET"); + + let v = loaded.expect("DA_VAULT_RESET must get an operator past a dead vault"); + assert!(v.is_empty()); + + // The unreadable original must not have been silently overwritten — + // it is the only thing anyone could still try to salvage. + let kept: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .flatten() + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n.starts_with("vault.json.corrupt-")) + .collect(); + assert_eq!(kept.len(), 1, "the original must be kept: {kept:?}"); + } + + #[test] + fn without_reset_a_dead_vault_still_refuses() { + let _guard = env_guard(); + std::env::remove_var("DA_VAULT_RESET"); + let dir = tempdir().unwrap(); + std::fs::write(dir.path().join("vault.json"), b"").unwrap(); + assert!(Vault::load(dir.path(), VaultKey::from_bytes(&[1u8; 32]).unwrap()).is_err()); + } + #[test] fn a_fresh_install_is_not_an_error() { let dir = tempdir().unwrap(); diff --git a/docs/releases/v0.1.4.md b/docs/releases/v0.1.4.md new file mode 100644 index 0000000..b58eca0 --- /dev/null +++ b/docs/releases/v0.1.4.md @@ -0,0 +1,52 @@ +## An accurate message, and a way out + +No functional change to how the vault is written or recovered. This fixes what +the service tells you when recovery is not possible, and adds an escape hatch +for the case it is not. + +### The message said the wrong thing + +When neither `vault.json` nor `vault.json.bak` could be loaded, the error read: + +> ...and the backup at /var/lib/discord-authenticator/vault.json.bak could not +> be used either. + +That is true for three quite different situations, and it reads as though a +backup exists. The case that actually bites is the one where **no backup was +ever created**: a vault that has not completed a single write since v0.1.3 +introduced backups has no `.bak` at all, so there is nothing on disk to recover +and no file to go looking for. The old wording sent you looking anyway. + +It now says which situation you are in: + +- `there is no backup at … — this vault has not been written since backups were + introduced, so none was ever created` +- `the backup at … exists but could not be parsed either` +- `the backup at … is empty as well, so there is nothing to recover` + +It also lists any `vault.json.corrupt-` files left by earlier +recoveries, since those are the only other thing worth inspecting. + +### DA_VAULT_RESET + +Refusing to start on an unreadable vault is still the default — booting empty +would look identical to a vault you had emptied on purpose, and the whole point +is that the service cannot tell the difference. But refusing must not be a dead +end, and having to edit files by hand inside a container that will not start is +a poor answer at 2am. + +Set `DA_VAULT_RESET=1` and the service boots with an empty vault instead. It +moves the unreadable file to `vault.json.corrupt-` *before* writing +the fresh one, so nothing is overwritten, and the log line names where it went. +Remove the variable afterwards: it has no effect on a vault that loads, but +leaving it set means the next genuinely-broken vault boots empty rather than +telling you about it. + +In the Unraid template: Docker → the container → **Add another Path, Variable, +Port or Label** → Variable, Key `DA_VAULT_RESET`, Value `1`. + +### Upgrading + +Nothing to do. Vault format unchanged, no accounts to re-import. If you are +currently stuck on the old message, this release does not recover the accounts +either — it just tells you why, and lets you start clean without leaving the UI.