From 2343905542e0739b1e94a29e4bb3402cbc368477 Mon Sep 17 00:00:00 2001 From: Jordan Smith Date: Wed, 22 Jul 2026 14:21:25 +1200 Subject: [PATCH 1/2] Secure history file with 0o600 rather than 0o644, do not store sensitive calls in repl (--api-key) --- src/cli/shell.rs | 172 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 170 insertions(+), 2 deletions(-) diff --git a/src/cli/shell.rs b/src/cli/shell.rs index f1442f4..ad7c9c0 100644 --- a/src/cli/shell.rs +++ b/src/cli/shell.rs @@ -10,8 +10,9 @@ use std::path::PathBuf; use clap::Parser; use reedline::{ - DefaultCompleter, DefaultHinter, FileBackedHistory, Highlighter, Prompt, PromptEditMode, - PromptHistorySearch, Reedline, Signal, StyledText, + DefaultCompleter, DefaultHinter, FileBackedHistory, Highlighter, History, HistoryItem, + HistoryItemId, HistorySessionId, Prompt, PromptEditMode, PromptHistorySearch, Reedline, + SearchQuery, Signal, StyledText, }; use crate::errors::{BitmexError, Result}; @@ -22,10 +23,12 @@ const HISTORY_CAPACITY: usize = 1_000; /// Run the interactive shell session. pub(crate) async fn run(ctx: &AppContext) -> Result<()> { let history_path = history_file()?; + secure_history_file(&history_path)?; let history = FileBackedHistory::with_file(HISTORY_CAPACITY, history_path) .map_err(|e| BitmexError::Config { message: format!("Failed to open shell history: {e}"), })?; + let history = RedactingHistory::new(history); let mut rl = Reedline::create() .with_history(Box::new(history)) @@ -258,6 +261,118 @@ fn history_file() -> Result { Ok(dir.join("history")) } +/// Ensure the shell history file exists with owner-only (0600) permissions. +/// +/// `reedline`'s `FileBackedHistory` opens the file via a plain `OpenOptions` +/// with no mode set, so a freshly created file would inherit the process +/// umask (commonly 0644). We create it ourselves first with `mode(0o600)`: +/// on Unix that mode is applied atomically by the `open(2)` syscall at the +/// moment of creation, so the file is never briefly world/group-readable. +/// `reedline` then just opens the already-existing, correctly-permissioned +/// file. A pre-existing file (e.g. from before this fix) is chmod'd, which +/// only tightens permissions it already had — no new exposure window. +#[cfg(unix)] +fn secure_history_file(path: &std::path::Path) -> Result<()> { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + + if path.exists() { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; + } else { + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path)?; + } + Ok(()) +} + +/// No-op on non-Unix platforms: we don't set an explicit ACL here. +/// +/// On Windows this relies on `%APPDATA%\bitmex` inheriting the user +/// profile's default NTFS permissions (owner + SYSTEM + Administrators, +/// not `Everyone`). That's an environmental default, not a guarantee this +/// function enforces — it can differ on domain-joined or shared machines. +#[cfg(not(unix))] +fn secure_history_file(_path: &std::path::Path) -> Result<()> { + Ok(()) +} + +/// Wraps `FileBackedHistory`, keeping lines that carry exchange credentials +/// out of the persisted history file entirely. +/// +/// `FileBackedHistory::save` only ever looks at `command_line` (session id, +/// exit status, etc. are discarded), so filtering on that field is enough. +/// Excluded entries are handed back to reedline unchanged instead of going +/// through `self.inner` — they never reach `self.inner`'s in-memory buffer, +/// so they can't be written to disk on sync/drop, and they also don't show +/// up in this session's up-arrow recall. +struct RedactingHistory { + inner: FileBackedHistory, +} + +impl RedactingHistory { + fn new(inner: FileBackedHistory) -> Self { + Self { inner } + } +} + +/// A line is sensitive if it invokes the `auth` command group, or passes +/// `--api-key`/`--api-secret` directly — both are global clap flags, so +/// they can appear on any command line, not just `auth set`. +fn is_sensitive(command_line: &str) -> bool { + let words = shell_words(command_line); + words.first().map(String::as_str) == Some("auth") + || words + .iter() + .any(|w| w == "--api-key" || w == "--api-secret") +} + +impl History for RedactingHistory { + fn save(&mut self, entry: HistoryItem) -> reedline::Result { + if is_sensitive(&entry.command_line) { + return Ok(entry); + } + self.inner.save(entry) + } + + fn load(&self, id: HistoryItemId) -> reedline::Result { + self.inner.load(id) + } + + fn count(&self, query: SearchQuery) -> reedline::Result { + self.inner.count(query) + } + + fn search(&self, query: SearchQuery) -> reedline::Result> { + self.inner.search(query) + } + + fn update( + &mut self, + id: HistoryItemId, + updater: &dyn Fn(HistoryItem) -> HistoryItem, + ) -> reedline::Result<()> { + self.inner.update(id, updater) + } + + fn clear(&mut self) -> reedline::Result<()> { + self.inner.clear() + } + + fn delete(&mut self, id: HistoryItemId) -> reedline::Result<()> { + self.inner.delete(id) + } + + fn sync(&mut self) -> std::io::Result<()> { + self.inner.sync() + } + + fn session(&self) -> Option { + self.inner.session() + } +} + fn print_shell_help() { println!("Available command groups:"); println!(" market, order, position, execution, wallet, staking, account"); @@ -322,4 +437,57 @@ mod tests { fn shell_words_single_quoted() { assert_eq!(shell_words("ticker 'XBT USD'"), vec!["ticker", "XBT USD"]); } + + #[test] + fn is_sensitive_detects_auth_group() { + assert!(is_sensitive("auth set --api-key foo --api-secret bar")); + assert!(is_sensitive("auth show")); + } + + #[test] + fn is_sensitive_detects_inline_secret_flags_on_any_command() { + assert!(is_sensitive( + "market instrument --symbol XBTUSD --api-secret bar" + )); + assert!(is_sensitive( + "order buy XBTUSD 100 --price 50000 --api-key foo" + )); + } + + #[test] + fn is_sensitive_leaves_ordinary_commands_alone() { + assert!(!is_sensitive("market instrument --symbol XBTUSD")); + assert!(!is_sensitive("account me")); + } + + #[test] + fn redacting_history_does_not_persist_sensitive_lines() { + let dir = + std::env::temp_dir().join(format!("bitmex-cli-test-history-{}", std::process::id())); + let path = dir.join("history"); + let _ = std::fs::create_dir_all(&dir); + let _ = std::fs::remove_file(&path); + + let inner = FileBackedHistory::with_file(100, path.clone()).unwrap(); + let mut history = RedactingHistory::new(inner); + + history + .save(HistoryItem::from_command_line( + "auth set --api-key foo --api-secret bar", + )) + .unwrap(); + history + .save(HistoryItem::from_command_line( + "market instrument --symbol XBTUSD", + )) + .unwrap(); + history.sync().unwrap(); + drop(history); + + let contents = std::fs::read_to_string(&path).unwrap_or_default(); + assert!(!contents.contains("api-secret")); + assert!(contents.contains("market instrument")); + + let _ = std::fs::remove_dir_all(&dir); + } } From bfe8f9b4c25101ab6400c2c0459e1f5e4b2f11cf Mon Sep 17 00:00:00 2001 From: Jordan Smith Date: Wed, 22 Jul 2026 14:32:21 +1200 Subject: [PATCH 2/2] Make sure everything bar --api-secret is persisted in history --- src/cli/shell.rs | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/cli/shell.rs b/src/cli/shell.rs index ad7c9c0..fb7f0f3 100644 --- a/src/cli/shell.rs +++ b/src/cli/shell.rs @@ -317,15 +317,14 @@ impl RedactingHistory { } } -/// A line is sensitive if it invokes the `auth` command group, or passes -/// `--api-key`/`--api-secret` directly — both are global clap flags, so -/// they can appear on any command line, not just `auth set`. +/// A line is sensitive if it contains `--api-secret` — which is a global flag, so +/// it can appear on any command line, not just `auth set`. fn is_sensitive(command_line: &str) -> bool { let words = shell_words(command_line); - words.first().map(String::as_str) == Some("auth") - || words + + words .iter() - .any(|w| w == "--api-key" || w == "--api-secret") + .any(|w| w == "--api-secret" || w.starts_with("--api-secret=")) } impl History for RedactingHistory { @@ -439,19 +438,31 @@ mod tests { } #[test] - fn is_sensitive_detects_auth_group() { + fn is_sensitive_detects_api_secret_on_auth_set() { assert!(is_sensitive("auth set --api-key foo --api-secret bar")); - assert!(is_sensitive("auth show")); } #[test] - fn is_sensitive_detects_inline_secret_flags_on_any_command() { + fn is_sensitive_detects_inline_secret_flag_on_any_command() { assert!(is_sensitive( "market instrument --symbol XBTUSD --api-secret bar" )); - assert!(is_sensitive( + } + + #[test] + fn is_sensitive_detects_equals_syntax() { + assert!(is_sensitive("auth set --api-secret=bar")); + } + + #[test] + fn is_sensitive_leaves_api_key_and_other_auth_commands_alone() { + // --api-key alone (no --api-secret) and non-secret auth subcommands + // are deliberately kept in history — only the secret is redacted. + assert!(!is_sensitive( "order buy XBTUSD 100 --price 50000 --api-key foo" )); + assert!(!is_sensitive("auth show")); + assert!(!is_sensitive("auth reset")); } #[test]