From a093f3c4767577e5963d2afac361298723b1be41 Mon Sep 17 00:00:00 2001 From: grom Date: Sat, 24 Oct 2026 15:43:06 -0600 Subject: [PATCH] app: the Remember Password box writes the encrypted blob MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asked for explicitly, a round after the machinery landed. `AutoLogin:: remember` is now the one write path the login screen uses, and it stores `password_encrypted` — the authenticated blob of `credential.rs`, keyed on 32 random bytes in a separate 0600 file. `migrate_plaintext` upgrades a file written by an older build on the next launch, and the plaintext field stays *readable* so that upgrade cannot lock anyone out of a store they already had. Two decisions inside it, both places where the convenient answer is the wrong one: A password that cannot be encrypted is **not stored at all** — there is no fallback to the clear field. The player cannot see the difference between a box that wrote a blob and one that quietly wrote plaintext, so a silent downgrade is worse than a box that visibly did not take. A stale plaintext field beside a blob is **cleared, not encrypted**: `effective_password` never read it, so encrypting it would promote a password the player has already replaced. What changed is the file. What did not is the threat model: the blob answers the file being read by another account, copied into a backup or pasted into a bug report, and cannot answer anything running as this user — the point of the feature is logging in without typing a secret, so no secret of the player's is in the chain to key it on. Both files are 0600 from their first byte. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SgstFWtn1RxEJ5kVrpgYwn --- crates/benilla-app/src/autologin.rs | 221 +++++++++++++++++++++++++--- crates/benilla-app/src/login/mod.rs | 15 +- docs/ADDON_COMPAT.md | 20 ++- 3 files changed, 226 insertions(+), 30 deletions(-) diff --git a/crates/benilla-app/src/autologin.rs b/crates/benilla-app/src/autologin.rs index bdc4f6af..38b555e4 100644 --- a/crates/benilla-app/src/autologin.rs +++ b/crates/benilla-app/src/autologin.rs @@ -21,16 +21,25 @@ //! //! **Not built**: the multi-account manager (the account button column, labels, //! autocomplete-by-letter, add/remove/reorder of accounts) — one account is what this store -//! holds, and the UI for the rest would be a second screen's worth of work. The mod's own -//! `ImportFile`/`ExportFile` persistence is SuperWoW's and also unbuilt; this writes its own -//! file. +//! holds, and the UI for the rest would be a second screen's worth of work. The mod persists +//! through SuperWoW's `ImportFile`/`ExportFile`, which this client now has +//! (`script/compat/nampower.rs`, fenced to `benilla-config/CustomFiles/`); this store still +//! writes its own file, because the fence deliberately cannot name it. //! -//! **The password is stored in plaintext**, which is the mod's behaviour and its own README's -//! warning. [`local_state::write_atomic_private`] keeps the file `0600`, so it is protected from -//! other users on the machine and from nothing else: anything running as this user can read it, -//! and it lands in any backup of the folder. Nothing is stored until the Remember Password box -//! is ticked, and unticking it clears what was stored, so a player who never opts in never has -//! a secret on disk. +//! **The password is stored encrypted** ([`crate::credential`]): an authenticated blob keyed on +//! 32 random bytes in a separate `0600` file, made by the same code nampower's `EncryptPassword` +//! and `EncryptedServerLogin` use. A plaintext password from an older build, or from a +//! hand-edited file, is upgraded in place the first time this store loads +//! ([`AutoLogin::migrate_plaintext`]). +//! +//! What that protects, stated so it is not mistaken for more: it answers the file being read by +//! another account on the machine, copied into a backup, or pasted into a bug report — the blob +//! is useless without the key file beside it. It does **not** protect against anything running +//! as this user, which reads the key as easily as the blob, and it cannot: the point of the +//! feature is logging in without typing a secret, so no secret of the player's is in the chain +//! to key it on. [`local_state::write_atomic_private`] keeps both files `0600` from their first +//! byte. Nothing is stored until the Remember Password box is ticked, and unticking it clears +//! both forms, so a player who never opts in never has a credential on disk. use std::collections::BTreeMap; @@ -53,16 +62,14 @@ pub(crate) struct AutoLogin { /// Account Name box) but kept separately: that file is the reference's `accountName` and is /// written by the stock checkbox, and tying the two would make unticking one clear the other. pub(crate) account: String, - /// **Plaintext.** Empty means no auto-login, whatever `enabled` says — see [`Self::armed`]. + /// **The older plaintext form**, kept only so a file written by a build before + /// [`Self::password_encrypted`] still logs in once. [`Self::migrate_plaintext`] empties it at + /// startup, so a current build leaves it empty and nothing new is ever written here. pub(crate) password: String, - /// The same credential as a blob, written by nampower's `EncryptedServerLogin` - /// (`crate::credential`). When present it **wins** over [`Self::password`], and the login - /// screen's refill is the only reader — [`Self::effective_password`] is that one read. - /// - /// Additive on purpose: the plaintext field stays exactly as it was, because storing the - /// password in plaintext was the player's own explicit choice and swapping it out from under - /// them is not what "implement these verbs" asked for. An addon (or a future tick-box) opts - /// in by writing this one; nothing migrates by itself. + /// The credential as an authenticated blob ([`crate::credential`]) — **what the Remember + /// Password box writes**, and what nampower's `EncryptedServerLogin` writes. When present it + /// wins over [`Self::password`], and the login screen's refill is the only reader: + /// [`Self::effective_password`] is that one read. pub(crate) password_encrypted: Option, /// Per-realm memory, keyed by the realm's name as the realm list gives it. pub(crate) realms: BTreeMap, @@ -110,6 +117,79 @@ impl AutoLogin { } } + /// Remember `account` and `password` as the Remember Password box's store — the password + /// **encrypted**. The one write path the login screen uses. + /// + /// A no-op when the stored credential already matches, so an ordinary login neither rewrites + /// the file nor re-rolls the blob's nonce; without that guard every login would look like a + /// change to anything watching the folder, and the file's mtime would stop meaning anything. + /// + /// When encryption is unavailable — no config home, or a key file that cannot be written — + /// **nothing is stored** and the warning says so. The alternative is falling back to the + /// plaintext field, which is the one outcome this box is no longer allowed to produce: a + /// silent downgrade is worse than a box that visibly did not take, because the player cannot + /// see the difference. + pub(crate) fn remember(&mut self, account: &str, password: &str) { + if self.account == account + && self.password.is_empty() + && self.password_encrypted.is_some() + && self.effective_password() == password + { + return; + } + let Some(blob) = crate::credential::encrypt(password) else { + warn!("autologin: no credential key could be made — the password 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(); + self.password.clear(); + self.password_encrypted = None; + self.save(); + return; + }; + self.store_encrypted(account, &blob); + } + + /// Upgrade a plaintext password from a build before [`Self::password_encrypted`] existed, or + /// from a hand-edited file. Called once at startup; answers whether the file changed, which + /// is what the tests read. + /// + /// A blob already present **wins**, which makes a non-empty plaintext field stale rather + /// than a second credential: it is cleared, not encrypted, because + /// [`Self::effective_password`] would never have read it and leaving it would be a secret on + /// disk that nothing uses. + /// + /// If encryption fails the plaintext is **put back**. Losing the password the player asked + /// this client to remember, because a key file could not be made, is a worse outcome than + /// leaving the file exactly as it was found. + pub(crate) fn migrate_plaintext(&mut self) -> bool { + if self.password.is_empty() { + return false; + } + if self.password_encrypted.is_some() { + self.password.clear(); + self.save(); + return true; + } + let pass = std::mem::take(&mut self.password); + match crate::credential::encrypt(&pass) { + Some(blob) => { + self.password_encrypted = Some(blob); + self.save(); + info!("autologin: the stored password was upgraded to the encrypted form"); + true + } + None => { + self.password = pass; + warn!( + "autologin: no credential key could be made — the stored password is still \ + in the older plaintext form" + ); + false + } + } + } + /// Store a credential as a blob, nampower's `EncryptedServerLogin`. The plaintext field is /// cleared, so the two never both hold a secret. pub(crate) fn store_encrypted(&mut self, account: &str, blob: &str) { @@ -253,6 +333,111 @@ mod tests { } } + /// A scoped config home, so a test's key file and store are its own. The lock is the env + /// guard's: `BENILLA_HOME` is process-wide, so two of these running at once would each see + /// the other's home. + type Guards = ( + std::sync::MutexGuard<'static, ()>, + local_state::test_env::EnvGuard, + local_state::test_env::EnvGuard, + ); + + fn scoped_home(tag: &str) -> (std::path::PathBuf, Guards) { + let lock = local_state::test_env::ENV_LOCK + .lock() + .unwrap_or_else(|e| e.into_inner()); + let tmp = std::env::temp_dir().join(format!("benilla-auto-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&tmp); + std::fs::create_dir_all(&tmp).expect("temp dir"); + let g = local_state::test_env::EnvGuard::set("BENILLA_HOME", tmp.to_str().unwrap()); + let c = local_state::test_env::EnvGuard::unset("WOW_CAPTURE"); + (tmp.clone(), (lock, g, c)) + } + + /// What the Remember Password box now writes. Asserted against the file's own bytes, because + /// the struct being right while the file holds the password would be the bug. + #[test] + fn remembering_a_password_writes_a_blob_and_not_the_password() { + let (tmp, _guards) = scoped_home("remember"); + + let mut auto = AutoLogin { + enabled: true, + ..Default::default() + }; + auto.remember("MYACCOUNT", "hunter2"); + + assert_eq!(auto.account, "MYACCOUNT"); + assert!(auto.password.is_empty(), "nothing is written in the clear"); + assert_eq!(auto.effective_password(), "hunter2", "and it reads back"); + + let text = std::fs::read_to_string(tmp.join("autologin.toml")).expect("the store"); + assert!( + !text.contains("hunter2"), + "autologin.toml must not contain the password:\n{text}" + ); + + // The same credential again must not re-roll the blob: an unchanged login is not a write. + let before = auto.password_encrypted.clone(); + auto.remember("MYACCOUNT", "hunter2"); + assert_eq!( + auto.password_encrypted, before, + "an unchanged credential leaves the blob alone" + ); + + // A changed password does replace it, and still decrypts to the new one. + auto.remember("MYACCOUNT", "hunter3"); + assert_ne!(auto.password_encrypted, before); + assert_eq!(auto.effective_password(), "hunter3"); + + let _ = std::fs::remove_dir_all(&tmp); + } + + /// A store written by a build before the blob existed: it logs in once, and the next launch + /// upgrades it in place. This is the path grom's own file takes. + #[test] + fn a_plaintext_password_is_upgraded_in_place() { + let (tmp, _guards) = scoped_home("migrate"); + + let mut auto = AutoLogin { + enabled: true, + account: "MYACCOUNT".into(), + password: "hunter2".into(), + ..Default::default() + }; + // Before the upgrade the old field is what refills — an older file still works. + assert_eq!(auto.effective_password(), "hunter2"); + + assert!(auto.migrate_plaintext(), "the file changed"); + assert!(auto.password.is_empty(), "the plaintext field is emptied"); + assert_eq!( + auto.effective_password(), + "hunter2", + "and the credential survives the upgrade" + ); + let text = std::fs::read_to_string(tmp.join("autologin.toml")).expect("the store"); + assert!( + !text.contains("hunter2"), + "the upgraded file must not contain the password:\n{text}" + ); + + // Idempotent: a second launch has nothing to do. + assert!(!auto.migrate_plaintext(), "nothing left to upgrade"); + + // A stale plaintext field beside a blob is cleared, not encrypted: `effective_password` + // never read it, so keeping it would be a secret on disk that nothing uses. + let mut both = AutoLogin { + enabled: true, + password: "stale".into(), + password_encrypted: auto.password_encrypted.clone(), + ..Default::default() + }; + assert!(both.migrate_plaintext()); + assert!(both.password.is_empty()); + assert_eq!(both.effective_password(), "hunter2", "the blob still wins"); + + let _ = std::fs::remove_dir_all(&tmp); + } + /// The file is hand-editable, so it has to survive a round trip unchanged. #[test] fn the_store_round_trips_through_toml() { diff --git a/crates/benilla-app/src/login/mod.rs b/crates/benilla-app/src/login/mod.rs index f9960b8a..fbc1d115 100644 --- a/crates/benilla-app/src/login/mod.rs +++ b/crates/benilla-app/src/login/mod.rs @@ -53,7 +53,14 @@ impl Plugin for LoginPlugin { fn build(&self, app: &mut App) { app.init_resource::() .init_resource::() - .insert_resource(crate::autologin::AutoLogin::load()) + .insert_resource({ + // 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. + let mut auto = crate::autologin::AutoLogin::load(); + auto.migrate_plaintext(); + auto + }) .add_systems(OnEnter(ClientState::Login), enter_login) .add_systems(OnExit(ClientState::Login), screen::exit_login) .add_systems( @@ -556,11 +563,7 @@ fn to_select_on_roster( // at submit time, which would persist a typo and then auto-submit it on every later launch. if auto.enabled { if let Some((user, pass)) = intent.creds.clone() { - if auto.account != user || auto.password != pass { - auto.account = user; - auto.password = pass; - auto.save(); - } + auto.remember(&user, &pass); } } intent.in_flight = false; diff --git a/docs/ADDON_COMPAT.md b/docs/ADDON_COMPAT.md index b6516daa..6f5b9c29 100644 --- a/docs/ADDON_COMPAT.md +++ b/docs/ADDON_COMPAT.md @@ -240,7 +240,7 @@ rebuilt in `autologin.rs` instead. | Area | Status | Notes | |---|---|---| | Remember the password | done | `autologin.rs` holds the store; `login::enter_login` fills both boxes when the screen comes up. **Fill only — deliberately not automatic.** The mod submits the form and enters the world for you; both halves of that were built here and then cut on the player's own call. An auto-login that lands you in the world is a client you cannot get a screen out of without knowing a trick, and the saving over a filled box is one click. The fill is gated on the tick as well as on the stored text, so unticking stops it in the same frame rather than waiting on the secret being cleared | -| Credential storage | done, **plaintext** | `benilla-config/autologin.toml`, written `0600` by `local_state::write_atomic_private` (asserted, including that it tightens a file that was already world-readable). Plaintext is the mod's own behaviour and its README's own warning; `0600` protects it from other users on the machine and from nothing else — anything running as this user can read it, and it lands in any backup of the folder. Its own file rather than a key in `config.toml`, because that is the file a player pastes into a bug report. Written **on the roster**, not at submit: a roster is the proof the credential was accepted, so a typo is never persisted and refilled on every later launch. Unticking the box **clears the password**, since that is the only way to get it off disk from inside the game | +| Credential storage | **superseded: encrypted** | `benilla-config/autologin.toml`, written `0600` by `local_state::write_atomic_private` (asserted, including that it tightens a file that was already world-readable). Plaintext is the mod's own behaviour and its README's own warning; `0600` protects it from other users on the machine and from nothing else — anything running as this user can read it, and it lands in any backup of the folder. Its own file rather than a key in `config.toml`, because that is the file a player pastes into a bug report. Written **on the roster**, not at submit: a roster is the proof the credential was accepted, so a typo is never persisted and refilled on every later launch. Unticking the box **clears the password**, since that is the only way to get it off disk from inside the game. **Superseded at the player's explicit request:** the box now writes `password_encrypted`, the authenticated blob of `crate::credential`, through `AutoLogin::remember`; `migrate_plaintext` upgrades an existing plaintext file in place on the next launch, and `effective_password` still reads the old field so a file written by an older build logs in once before it is upgraded. What changed is the file; what did not is the threat model — see the credential section below for what the blob answers and what it cannot | | Remember the last character | done, **selects only** | `char_select::apply_roster_policy`. The roster is always shown — nothing enters the world for the player. Remembered by **name**, and it outranks 1.12's own `lastCharacter` CVar, which is not a preference but a correctness fix: that CVar is a row *index*, and this screen now lets the player reorder rows, so after a reorder the index names whoever moved into that slot. A name survives reordering; an index does not. A remembered character who is gone (deleted, or another realm) falls back to the stock row. Recorded in `select_input` — the path that means *the player chose* — so a reconnect's re-pick and the env fast paths cannot rewrite it | | Login screen cleanup | done | `cleanLoginScreen` (CVar, default **1**) drops the Blizzard logo, the `BLIZZ_DISCLAIMER` line and the version string. That is the whole of what this screen had left to hide: the rest of the mod's list — the Turtle website/armory/forum/Discord/Reddit buttons, Credits, Cinematics, Terms of Use, ESRB, Manage Account, Community Site, server news — **this screen never drew**, because `login/` keeps only what logging in needs (its module doc says so, and predates this work). Read once in `enter_login` and latched on `LoginForm`, not read per frame where it is used: the latter measured at six undeclared orders against every writer of the registry, for a value that cannot change while the screen is up (there is no console at the login screen to write it from) | | Character row reordering | done | The mod's per-realm `order` + `char_buttons_locked`. `autologin::apply_order` is a permutation by construction: saved names first in their saved order, anything unmentioned keeping its server order on the end, so a character created since the order was saved appears at the bottom rather than vanishing and one deleted elsewhere is skipped rather than leaving a hole. Applied in `apply_roster_policy` *before* anything indexes a row, because the remembered-row CVar, `select_created_by_name` and `WOW_CHARSELECT_PICK` all mean rows as the player sees them. The arrows fold into `refresh_list`'s existing row loop (they are `Button`s carrying a `SelectAction`, so that query already walked past them) rather than a second `Visibility` writer. A move keeps the selection on the *character*, not the row — swapping rows under a highlighted name would silently re-aim Enter World at its neighbour | @@ -614,11 +614,19 @@ key file generated here would not decrypt one made there whatever the cipher. A real install answers nil, which the drain reports rather than storing — a stored credential that cannot be read back is a login box that silently refills empty. -**The existing plaintext path is untouched.** Storing the password in plaintext was grom's own -explicit choice when the alternatives were a keyring or nothing; this is additive, and nothing -migrates by itself. The machinery to make the Remember Password tick-box write a blob instead is -now all here — one line in `login/mod.rs` — and that is his call to make, not a side effect of -porting two verbs. +**The tick-box now writes the blob** — asked for explicitly, a round after the machinery landed. +Storing the password in plaintext had been grom's own choice when the alternatives were a keyring +or nothing, so the round that built these two verbs left that path alone and said what switching +it would cost; this is the switch, not a side effect of the port. `AutoLogin::remember` is the one +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. + +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 +stored plaintext, so a silent downgrade is worse than a box that visibly did not take. And **a +stale plaintext field beside a blob is cleared rather than encrypted**: `effective_password` never +read it, so encrypting it would promote a password the player has already replaced. **Everything below this line is the reasoning of earlier rounds, kept rather than deleted.** Each section records what was missing *then*, what it would have taken, and — where a later round built