app: seal a password only if it reads back, and keep tests out of the store

This cost grom his stored password once. Two unit tests ran at the same
time: one redirected the config directory through `BENILLA_HOME`, which
is process-wide, while the other stood up `LoginPlugin`, whose `build`
had just gained a call to `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 replacing it does not
decrypt. He types the password once more and `remember` stores it
properly, since a blob that answers empty does not match and is resealed.

Two fixes, and the second is the one that matters.

The migration is now `#[cfg(not(test))]`: it was the only line in that
plugin's build that writes to the player's own config directory, and no
test has any business there.

`autologin::sealed` encrypts and then **reads the blob back**, and
refuses to clear the plaintext unless the round trip holds. That would
have prevented this on its own, and it closes the general hole rather
than this instance of it: `credential.rs` already carries a note about a
scheme that round-trips with itself while being wrong, and this is the
same mistake one level up — the cipher was right, the *key* was what
disagreed, and the only readable copy of the password was thrown away
before anything checked.

A test drives the keep-it branch through the hermetic capture flag, the
one state where sealing honestly cannot work, and asserts the plaintext
is still there afterwards. An ignored `live_check` test reads the real
store by hand and prints lengths only, never the password; it is what
caught this.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn

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<String> {
+    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"
+        );
+    }
+}

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
             })

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