diff --git a/crates/benilla-app/src/autologin.rs b/crates/benilla-app/src/autologin.rs index 38b555e4..b570b1c2 100644 --- a/crates/benilla-app/src/autologin.rs +++ b/crates/benilla-app/src/autologin.rs @@ -102,6 +102,27 @@ impl Default for RealmMemory { } } +/// Encrypt `password` **and read it back**, answering `None` unless the round trip holds. +/// +/// The check is the point. `credential::encrypt` writes a blob against the key file it finds, and +/// a caller that then clears the plaintext has destroyed the only readable copy — so if the blob +/// and the key ever disagree, the password is simply gone, and the first anyone learns of it is a +/// login box that refills empty. +/// +/// That is not hypothetical: it happened to the player's own store during this feature's own +/// development. Two unit tests ran concurrently, one of them redirecting the config directory +/// through a process-wide environment variable, and a blob written against one key landed in a +/// file beside another. A round trip checked here would have refused to clear the plaintext and +/// the password would have survived. +fn sealed(password: &str) -> Option { + let blob = crate::credential::encrypt(password)?; + if crate::credential::decrypt(&blob).as_deref() != Some(password) { + warn!("autologin: the sealed password did not read back — keeping what was there"); + return None; + } + Some(blob) +} + impl AutoLogin { /// The password to refill the login box with: the blob decrypted when there is one, else the /// plaintext field. @@ -137,8 +158,8 @@ impl AutoLogin { { return; } - let Some(blob) = crate::credential::encrypt(password) else { - warn!("autologin: no credential key could be made — the password was not stored"); + let Some(blob) = sealed(password) else { + warn!("autologin: the password could not be sealed — it was not stored"); // The account is still worth keeping; any older secret is not. A password that // cannot be encrypted must not be left behind in the plaintext field either. self.account = account.to_string(); @@ -172,7 +193,7 @@ impl AutoLogin { return true; } let pass = std::mem::take(&mut self.password); - match crate::credential::encrypt(&pass) { + match sealed(&pass) { Some(blob) => { self.password_encrypted = Some(blob); self.save(); @@ -182,8 +203,8 @@ impl AutoLogin { None => { self.password = pass; warn!( - "autologin: no credential key could be made — the stored password is still \ - in the older plaintext form" + "autologin: the password could not be sealed — the stored one is still in \ + the older plaintext form" ); false } @@ -438,6 +459,38 @@ mod tests { let _ = std::fs::remove_dir_all(&tmp); } + /// The branch that cost the player their stored password once: when the credential cannot be + /// sealed, the plaintext must be **put back**, not dropped. Driven here by the hermetic + /// capture flag, which is the one state in which `encrypt` honestly cannot work. + #[test] + fn a_password_that_cannot_be_sealed_is_kept_rather_than_lost() { + let _lock = local_state::test_env::ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let _c = local_state::test_env::EnvGuard::set("WOW_CAPTURE", "1"); + + assert!( + sealed("hunter2").is_none(), + "no config home, so there is no key to seal against" + ); + let mut auto = AutoLogin { + enabled: true, + account: "MYACCOUNT".into(), + password: "hunter2".into(), + ..Default::default() + }; + assert!(!auto.migrate_plaintext(), "nothing changed"); + assert_eq!( + auto.password, "hunter2", + "the only readable copy must still be there" + ); + assert_eq!(auto.effective_password(), "hunter2"); + + // And `remember` does not leave a blob nothing can read. + auto.remember("MYACCOUNT", "hunter3"); + assert!(auto.password_encrypted.is_none()); + } + /// The file is hand-editable, so it has to survive a round trip unchanged. #[test] fn the_store_round_trips_through_toml() { @@ -472,3 +525,32 @@ mod tests { ); } } + +/// A one-off check of the player's *real* store, run by hand: `cargo test -p benilla-app --lib +/// the_live_store_still_decrypts -- --ignored --nocapture`. Ignored because it reads the real +/// config directory, which no ordinary test may do. +#[cfg(test)] +mod live_check { + /// Does the blob in the real `autologin.toml` still decrypt? Prints the length only — never + /// the password, which is the whole point of the file. + #[test] + #[ignore = "reads the real benilla-config"] + fn the_live_store_still_decrypts() { + let auto = super::AutoLogin::load(); + println!( + "enabled={} account_len={} plaintext_len={} blob={}", + auto.enabled, + auto.account.len(), + auto.password.len(), + auto.password_encrypted + .as_deref() + .map_or("none".to_string(), |b| format!("{} chars", b.len())) + ); + let pw = auto.effective_password(); + println!("effective_password length = {}", pw.len()); + assert!( + !pw.is_empty() || auto.password_encrypted.is_none(), + "a stored blob that answers empty means the login box will refill empty" + ); + } +} diff --git a/crates/benilla-app/src/login/mod.rs b/crates/benilla-app/src/login/mod.rs index fbc1d115..50d96e9b 100644 --- a/crates/benilla-app/src/login/mod.rs +++ b/crates/benilla-app/src/login/mod.rs @@ -57,7 +57,17 @@ impl Plugin for LoginPlugin { // The upgrade of a plaintext password from an older build runs here rather than // inside `load`, so loading the store stays a read and the rewrite happens // exactly once per launch. + // + // **Never under a test.** This is the one line in the plugin's build that writes + // to the player's own config directory, and a unit test that stands the plugin + // up would rewrite their credential file — which is exactly what happened while + // this was being built: a concurrent test had redirected `BENILLA_HOME`, so the + // blob and the key file came from different places and the password was lost. + // `autologin::sealed` now refuses that write; this keeps the test out of the + // file at all, which is the better of the two fixes and not a substitute for it. + #[cfg_attr(test, allow(unused_mut))] let mut auto = crate::autologin::AutoLogin::load(); + #[cfg(not(test))] auto.migrate_plaintext(); auto }) diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index b7208845..814bf52f 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -621,6 +621,22 @@ it would cost; this is the switch, not a side effect of the port. `AutoLogin::re write path, `migrate_plaintext` upgrades a file written by an older build, and the plaintext field stays *readable* so that upgrade cannot lock anyone out of their own store. +**It cost grom his stored password once, and that is worth writing down.** While this was being +built, two unit tests ran concurrently: one redirected the config directory through +`BENILLA_HOME`, which is process-wide, and the other stood up `LoginPlugin`, whose `build` now +called `migrate_plaintext`. So his real `autologin.toml` was upgraded against a key file that was +not the one beside it — the plaintext was cleared and the blob it was replaced with does not +decrypt. He has to type the password once more; `remember` then stores it correctly, because a +blob that answers empty does not match and is re-sealed. + +Two fixes, and the second is the one that matters: the migration is now `#[cfg(not(test))]`, so a +test cannot write to the player's own config at all; and **`sealed` encrypts and then reads the +blob back, refusing to clear the plaintext unless the round trip holds**. The second would have +prevented it on its own. The general shape of the mistake is the one `credential.rs` already +carries a note about — a scheme that round-trips with *itself* while being wrong — one level up: +here the cipher was right and the *key* was the thing that disagreed, and the only copy of the +password was thrown away before anyone checked. + Two decisions inside it worth naming, because both are places a convenient answer is the wrong one. **A password that cannot be encrypted is not stored at all** — no fallback to the clear field: the player cannot see the difference between a box that stored a blob and one that quietly