diff --git a/CHANGELOG.md b/CHANGELOG.md index f000043b..0bb77185 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **`--password` prompt is now collected once up-front and shared across all parallel connection tasks** (#201, closes #200). Previously each per-node SSH task prompted for the password independently, racing each other for stdin and interleaving with the progress UI. The dispatcher now collects the password before any executor or `indicatif` UI is initialized and threads an `Arc` to every per-node auth task — matching the existing `-S` (`SudoPassword`) pattern. The `BSSH_PASSWORD` environment variable is supported for automation scenarios (discouraged; see security notes below). -- **`-S` / `--sudo-password` warning now routes to stderr** when passed to subcommands where it has no effect (`ping`, `upload`, `download`, `list`), keeping stdout clean for scripts that consume bssh output (#201). +- **`-S` / `--sudo-password` warning now routes to stderr** when passed to subcommands where it has no effect (`ping`, `upload`, `download`, `list`, `cache-stats`, and interactive shells), keeping stdout clean for scripts that consume bssh output (#201, follow-up to #200). ## [2.1.4] - 2026-05-10 diff --git a/crates/bssh-russh-sftp/src/client/session.rs b/crates/bssh-russh-sftp/src/client/session.rs index 6d195b08..0fe16728 100644 --- a/crates/bssh-russh-sftp/src/client/session.rs +++ b/crates/bssh-russh-sftp/src/client/session.rs @@ -190,7 +190,7 @@ impl SftpSession { .files .into_iter() .map(|f| (f.filename, f.attrs)) - .chain(files.into_iter()) + .chain(files) .collect(); } Err(Error::Status(status)) if status.status_code == StatusCode::Eof => break, diff --git a/src/app/dispatcher.rs b/src/app/dispatcher.rs index 419e7fc5..4eee8938 100644 --- a/src/app/dispatcher.rs +++ b/src/app/dispatcher.rs @@ -83,25 +83,40 @@ fn build_ssh_connection_config( ssh_connection_config } -/// Decide whether `-S` (sudo-password) is meaningful for the given subcommand. +/// Decide whether `-S` (sudo-password) is meaningful for the given dispatch path. /// -/// `exec` and the interactive paths legitimately need a sudo password; the -/// other subcommands run either no command (`ping` just runs `true`) or operate -/// over SFTP (`upload`, `download`, `list`), so sudo is never relevant. -fn sudo_password_is_applicable(command: &Option) -> bool { +/// Only command execution can consume `SudoPassword`, because it monitors +/// command output for sudo prompts and injects the password into the remote +/// process. Interactive shells handle their own terminal input, while ping, +/// SFTP, list, and cache-stat paths never inspect remote sudo prompts. +fn sudo_password_is_applicable(command: &Option, command_text: &str) -> bool { match command { Some(Commands::Ping) | Some(Commands::Upload { .. }) | Some(Commands::Download { .. }) | Some(Commands::List) + | Some(Commands::Interactive { .. }) | Some(Commands::CacheStats { .. }) => false, - // exec (None), Interactive, and unknown future subcommands default to - // "applicable" — better to over-accept than silently strip a flag the - // user explicitly passed. - _ => true, + // `None` is the exec/SSH-compatibility path. A non-empty command can + // use sudo injection; an empty command is an interactive SSH shell and + // has no sudo-injection hook. + None => !command_text.is_empty(), } } +/// Decide whether `--password` should be collected for this dispatcher path. +/// +/// Most paths create SSH connections and can use an SSH password. List and +/// cache-stat paths do not connect to a remote host, so prompting would be a +/// surprising no-op if `dispatch_command()` is called directly in tests or +/// library-like contexts (the binary entry point handles those paths earlier). +fn ssh_password_is_applicable(command: &Option) -> bool { + !matches!( + command, + Some(Commands::List) | Some(Commands::CacheStats { .. }) + ) +} + /// Human-readable name of the currently-dispatched subcommand for diagnostics. fn subcommand_name(command: &Option) -> &'static str { match command { @@ -132,17 +147,24 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { ); } - // Warn if -S is passed to subcommands where it has no effect. We choose + // Warn if -S is passed to dispatch paths where it has no effect. We choose // a warning (not a hard reject) to match typical CLI UX: existing scripts // that happen to pass -S to `ping` or `upload` keep working, but the user // gets visible feedback that the flag is being ignored. - if cli.sudo_password && !sudo_password_is_applicable(&cli.command) { + if cli.sudo_password && !sudo_password_is_applicable(&cli.command, &command) { eprintln!( "Warning: --sudo-password (-S) has no effect for the `{}` subcommand and will be ignored", subcommand_name(&cli.command) ); } + if cli.password && !ssh_password_is_applicable(&cli.command) { + eprintln!( + "Warning: --password has no effect for the `{}` subcommand and will be ignored", + subcommand_name(&cli.command) + ); + } + // Hoist `--password` collection: prompt ONCE here, before any per-node // SSH connection task is spawned and before the indicatif progress UI is // rendered. Previously this prompt was issued inside each parallel auth @@ -154,13 +176,14 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { // The returned value is wrapped in `Arc` so cloning across // per-node tasks does not duplicate the underlying secret; on drop, the // memory is zeroized by `secrecy::SecretString`. - let ssh_password: Option> = if cli.password { - Some(Arc::new(get_password(true).map_err(|e| { - anyhow::anyhow!("Failed to collect SSH password: {e}") - })?)) - } else { - None - }; + let ssh_password: Option> = + if cli.password && ssh_password_is_applicable(&cli.command) { + Some(Arc::new(get_password(true).map_err(|e| { + anyhow::anyhow!("Failed to collect SSH password: {e}") + })?)) + } else { + None + }; // Calculate hostname for SSH config integration let hostname_for_ssh_config = if cli.is_ssh_mode() { @@ -604,3 +627,49 @@ async fn handle_exec_command( execute_command(params).await } } + +#[cfg(test)] +mod tests { + use super::*; + + fn interactive_command() -> Option { + Some(Commands::Interactive { + single_node: false, + multiplex: true, + prompt_format: "[{node}:{user}@{host}:{pwd}]$ ".to_string(), + history_file: PathBuf::from("~/.bssh_history"), + work_dir: None, + }) + } + + #[test] + fn sudo_password_applies_only_to_exec_commands() { + assert!(sudo_password_is_applicable(&None, "uptime")); + + assert!(!sudo_password_is_applicable(&None, "")); + assert!(!sudo_password_is_applicable(&Some(Commands::Ping), "true")); + assert!(!sudo_password_is_applicable(&interactive_command(), "")); + assert!(!sudo_password_is_applicable( + &Some(Commands::CacheStats { + detailed: false, + clear: false, + maintain: false, + }), + "", + )); + } + + #[test] + fn ssh_password_is_not_collected_for_local_only_subcommands() { + assert!(ssh_password_is_applicable(&None)); + assert!(ssh_password_is_applicable(&Some(Commands::Ping))); + assert!(ssh_password_is_applicable(&interactive_command())); + + assert!(!ssh_password_is_applicable(&Some(Commands::List))); + assert!(!ssh_password_is_applicable(&Some(Commands::CacheStats { + detailed: false, + clear: false, + maintain: false, + }))); + } +}