diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a112093eaf..b3a29b66dc2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,8 @@ ### Added +- [#6012](https://github.com/ChainSafe/forest/issues/6012): Stricter validation of address arguments in `forest-wallet` subcommands. + ### Changed ### Removed diff --git a/src/wallet/main.rs b/src/wallet/main.rs index e001077ea56..8327c7945f0 100644 --- a/src/wallet/main.rs +++ b/src/wallet/main.rs @@ -14,6 +14,14 @@ pub async fn main(args: impl IntoIterator) -> anyhow::Result< where ArgT: Into + Clone, { + // Preliminary client without the token to check network. This needs to occur before parsing to ensure the `StrictAddress` validation works correctly. + let client = rpc::Client::default_or_from_env(None)?; + if let Ok(name) = StateNetworkName::call(&client, ()).await + && !matches!(NetworkChain::from_str(&name), Ok(NetworkChain::Mainnet)) + { + CurrentNetwork::set_global(Network::Testnet); + } + // Capture Cli inputs let Cli { opts, @@ -24,11 +32,6 @@ where let client = rpc::Client::default_or_from_env(opts.token.as_deref())?; - let name = StateNetworkName::call(&client, ()).await?; - let chain = NetworkChain::from_str(&name)?; - if chain.is_testnet() { - CurrentNetwork::set_global(Network::Testnet); - } // Run command cmd.run(client, remote_wallet, encrypt).await } diff --git a/src/wallet/subcommands/mod.rs b/src/wallet/subcommands/mod.rs index c99adacf7af..ae465e47fde 100644 --- a/src/wallet/subcommands/mod.rs +++ b/src/wallet/subcommands/mod.rs @@ -29,3 +29,29 @@ pub struct Cli { #[command(subcommand)] pub cmd: wallet_cmd::WalletCommands, } + +#[cfg(test)] +mod tests { + use super::Cli; + use clap::Parser; + use clap::error::ErrorKind; + use rstest::rstest; + + fn try_parse(args: &[&str]) -> Result { + let argv = std::iter::once("forest-wallet").chain(args.iter().copied()); + Cli::try_parse_from(argv).map_err(|e| e.kind()) + } + + #[rstest] + #[case::balance(&["balance", "not-an-address"])] + #[case::export(&["export", "not-an-address"])] + #[case::has(&["has", "not-an-address"])] + #[case::set_default(&["set-default", "not-an-address"])] + #[case::delete(&["delete", "not-an-address"])] + #[case::sign(&["sign", "-m", "de", "-a", "not-an-address"])] + #[case::verify(&["verify", "-a", "not-an-address", "-m", "de", "-s", "de"])] + #[case::send_from(&["send", "--from", "not-an-address", "f01234", "1"])] + fn migrated_subcommands_reject_malformed_address(#[case] args: &[&str]) { + assert_eq!(try_parse(args).err(), Some(ErrorKind::ValueValidation)); + } +} diff --git a/src/wallet/subcommands/wallet_cmd.rs b/src/wallet/subcommands/wallet_cmd.rs index f771a3ebcea..9796573a3c6 100644 --- a/src/wallet/subcommands/wallet_cmd.rs +++ b/src/wallet/subcommands/wallet_cmd.rs @@ -149,13 +149,11 @@ impl WalletBackend { } } - async fn wallet_default_address(&self) -> anyhow::Result> { + async fn wallet_default_address(&self) -> anyhow::Result> { if let Some(keystore) = &self.local { - Ok(crate::key_management::get_default(keystore)?.map(|s| s.to_string())) + Ok(crate::key_management::get_default(keystore)?) } else { - Ok(WalletDefaultAddress::call(&self.remote, ()) - .await? - .map(|it| it.to_string())) + Ok(WalletDefaultAddress::call(&self.remote, ()).await?) } } @@ -211,7 +209,7 @@ pub enum WalletCommands { /// Get account balance Balance { /// The address of the account to check - address: String, + address: StrictAddress, /// Output is rounded to 4 significant figures by default. /// Do not round // ENHANCE(aatifsyed): add a --round/--no-round argument pair @@ -227,12 +225,12 @@ pub enum WalletCommands { /// Export the wallet's keys Export { /// The address that contains the keys to export - address: String, + address: StrictAddress, }, /// Check if the wallet has a key Has { /// The key to check - key: String, + key: StrictAddress, }, /// Import keys from existing wallet Import { @@ -254,7 +252,7 @@ pub enum WalletCommands { /// Set the default wallet address SetDefault { /// The given key to set to the default address - key: String, + key: StrictAddress, }, /// Sign a message Sign { @@ -263,7 +261,7 @@ pub enum WalletCommands { message: String, /// The address to be used to sign the message #[arg(short)] - address: String, + address: StrictAddress, }, /// Validates whether a given string can be decoded as a well-formed address ValidateAddress { @@ -275,7 +273,7 @@ pub enum WalletCommands { Verify { /// The address used to sign the message #[arg(short)] - address: String, + address: StrictAddress, /// The message to verify #[arg(short)] message: String, @@ -286,14 +284,18 @@ pub enum WalletCommands { /// Deletes the wallet associated with the given address. Delete { /// The address of the wallet to delete - address: String, + address: StrictAddress, }, /// Send funds between accounts Send { /// optionally specify the account to send funds from (otherwise the default /// one will be used) #[arg(long)] - from: Option, + from: Option, + /// The recipient address. Accepts either a FIL address (e.g. + /// `f1.../t1...`) or an ETH address (e.g. `0x...`). + // Kept as `String` rather than `StrictAddress` because the latter + // rejects the ETH form, which `resolve_target_address` handles. target_address: String, #[arg(value_parser = humantoken::parse)] amount: TokenAmount, @@ -329,9 +331,7 @@ impl WalletCommands { no_round, no_abbrev, } => { - let StrictAddress(address) = StrictAddress::from_str(&address) - .with_context(|| format!("Invalid address: {address}"))?; - let balance = WalletBalance::call(&backend.remote, (address,)).await?; + let balance = WalletBalance::call(&backend.remote, (address.into(),)).await?; println!("{}", format_balance(&balance, no_round, no_abbrev)); Ok(()) } @@ -343,27 +343,21 @@ impl WalletCommands { println!("{default_addr}"); Ok(()) } - Self::Export { - address: address_string, - } => { - let StrictAddress(address) = StrictAddress::from_str(&address_string) - .with_context(|| format!("Invalid address: {address_string}"))?; - let key_info = backend.wallet_export(address).await?; + Self::Export { address } => { + let key_info = backend.wallet_export(address.into()).await?; let encoded_key = key_info.into_lotus_json_string()?; println!("{}", hex::encode(encoded_key)); Ok(()) } Self::Has { key } => { - let StrictAddress(address) = StrictAddress::from_str(&key) - .with_context(|| format!("Invalid address: {key}"))?; - - println!("{response}", response = backend.wallet_has(address).await?); + println!( + "{response}", + response = backend.wallet_has(key.into()).await? + ); Ok(()) } Self::Delete { address } => { - let StrictAddress(address) = StrictAddress::from_str(&address) - .with_context(|| format!("Invalid address: {address}"))?; - + let address: Address = address.into(); backend.wallet_delete(address).await?; println!("deleted {address}."); Ok(()) @@ -407,13 +401,9 @@ impl WalletCommands { no_round, no_abbrev, } => { - let (key_pairs, default) = + let (key_pairs, default_address) = tokio::try_join!(backend.list_addrs(), backend.wallet_default_address(),)?; - let default_address = default - .as_deref() - .and_then(|s| StrictAddress::from_str(s).ok().map(Into::into)); - let remote = &backend.remote; let results = futures::future::join_all(key_pairs.iter().copied().map(|a| async move { @@ -455,20 +445,12 @@ impl WalletCommands { println!("{list}"); Ok(()) } - Self::SetDefault { key } => { - let StrictAddress(key) = StrictAddress::from_str(&key) - .with_context(|| format!("Invalid address: {key}"))?; - - backend.wallet_set_default(key).await - } + Self::SetDefault { key } => backend.wallet_set_default(key.into()).await, Self::Sign { address, message } => { - let StrictAddress(address) = StrictAddress::from_str(&address) - .with_context(|| format!("Invalid address: {address}"))?; - let message = hex::decode(message).context("Message has to be a hex string")?; let message = BASE64_STANDARD.encode(message); - let signature = backend.wallet_sign(address, message).await?; + let signature = backend.wallet_sign(address.into(), message).await?; println!("{}", hex::encode(signature.to_bytes())); Ok(()) } @@ -484,12 +466,12 @@ impl WalletCommands { } => { let sig_bytes = hex::decode(signature).context("Signature has to be a hex string")?; - let StrictAddress(address) = StrictAddress::from_str(&address) - .with_context(|| format!("Invalid address: {address}"))?; let msg = hex::decode(message).context("Message has to be a hex string")?; let signature = Signature::from_bytes(sig_bytes)?; - let is_valid = backend.wallet_verify(address, msg, signature).await?; + let is_valid = backend + .wallet_verify(address.into(), msg, signature) + .await?; println!("{is_valid}"); Ok(()) @@ -502,13 +484,11 @@ impl WalletCommands { gas_limit, gas_premium, } => { - let from: Address = if let Some(from) = from { - StrictAddress::from_str(&from)?.into() - } else { - StrictAddress::from_str(&backend.wallet_default_address().await?.context( + let from: Address = match from { + Some(a) => a.into(), + None => backend.wallet_default_address().await?.context( "No default wallet address selected. Please set a default address.", - )?)? - .into() + )?, }; let (mut to, is_0x_recipient) = resolve_target_address(&target_address)?;