fix: 405 method not allowed errors caused by trailing forward-slashes (#1348)

## 🎟️ Tracking

https://bitwarden.atlassian.net/browse/SM-1162

## 📔 Objective

Trim trailing forward-slashes on server URLs to avoid an ambiguous `405
method not allowed` error in our CLI. All methods of input should be
handled:

- using the `--server-url` arg
- using the `BWS_SERVER_URL` environment variable
- reading from the `server-{base,api,identity}` keys in the config file
- _writing_ to the config file with `bws config set
server-{base,api,identity}`

In any of those cases, the changes here should drop any trailing
forward-slashes successfully.

## ⏰ Reminders before review

- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
  team

## 🦮 Reviewer guidelines

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- 👍 (`:+1:`) or similar for great changes
- 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info
- ❓ (`:question:`) for questions
- 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry
that's not quite a confirmed
  issue and could potentially benefit from discussion
- 🎨 (`:art:`) for suggestions / improvements
- ❌ (`:x:`) or ⚠️ (`:warning:`) for more significant problems or
concerns needing attention
- 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or
indications of technical debt
- ⛏ (`:pick:`) for minor or nitpick changes

Author: tangowithfoxtrot

crates/bws/src/cli.rs

@@ -1,7 +1,7 @@
 use std::path::PathBuf;
 
 use bitwarden_cli::Color;
-use clap::{ArgGroup, Parser, Subcommand, ValueEnum};
+use clap::{builder::ValueParser, ArgGroup, Parser, Subcommand, ValueEnum};
 use clap_complete::Shell;
 use uuid::Uuid;
 
@@ -63,10 +63,18 @@ pub(crate) struct Cli {
     #[arg(short = 'p', long, global = true, env = PROFILE_KEY_VAR_NAME, help="Profile to use from the config file")]
     pub(crate) profile: Option<String>,
 
-    #[arg(short = 'u', long, global = true, env = SERVER_URL_KEY_VAR_NAME, help="Override the server URL from the config file")]
+    #[arg(short = 'u', long, global = true, env = SERVER_URL_KEY_VAR_NAME, help="Override the server URL from the config file", value_parser = ValueParser::new(url_parser) )]
     pub(crate) server_url: Option<String>,
 }
 
+fn url_parser(value: &str) -> Result<String, String> {
+    if value.starts_with("http://") || value.starts_with("https://") {
+        Ok(value.trim_end_matches('/').into())
+    } else {
+        Err(format!("'{value}' is not a valid URL"))
+    }
+}
+
 #[derive(Subcommand, Debug)]
 pub(crate) enum Commands {
     #[command(long_about = "Configure the CLI", arg_required_else_help(true))]

crates/bws/src/config.rs

@@ -17,15 +17,35 @@ pub(crate) struct Config {
 
 #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, Default)]
 pub(crate) struct Profile {
+    #[serde(deserialize_with = "deserialize_trimmed_url", default)]
     pub server_base: Option<String>,
+    #[serde(deserialize_with = "deserialize_trimmed_url", default)]
     pub server_api: Option<String>,
+    #[serde(deserialize_with = "deserialize_trimmed_url", default)]
     pub server_identity: Option<String>,
     pub state_dir: Option<String>,
     pub state_opt_out: Option<String>,
 }
 
+fn deserialize_trimmed_url<'de, D>(deserializer: D) -> Result<Option<String>, D::Error>
+where
+    D: serde::Deserializer<'de>,
+{
+    let opt_string: Option<String> = Option::deserialize(deserializer)?;
+    Ok(opt_string.map(|s| s.trim_end_matches('/').to_string()))
+}
+
 impl ProfileKey {
     fn update_profile_value(&self, p: &mut Profile, value: String) {
+        let value = if matches!(
+            self,
+            ProfileKey::server_base | ProfileKey::server_api | ProfileKey::server_identity
+        ) {
+            value.trim_end_matches('/').to_string()
+        } else {
+            value
+        };
+
         match self {
             ProfileKey::server_base => p.server_base = Some(value),
             ProfileKey::server_api => p.server_api = Some(value),
@@ -90,7 +110,12 @@ pub(crate) fn update_profile(
     let mut config = load_config(config_file, false)?;
 
     let p = config.profiles.entry(profile).or_default();
-    name.update_profile_value(p, value);
+
+    if value.starts_with("http://") || value.starts_with("https://") {
+        name.update_profile_value(p, value.trim_end_matches('/').to_string());
+    } else {
+        name.update_profile_value(p, value);
+    }
 
     write_config(config, config_file)?;
     Ok(())
@@ -123,6 +148,7 @@ impl Profile {
             state_opt_out: None,
         })
     }
+
     pub(crate) fn api_url(&self) -> Result<String> {
         if let Some(api) = &self.server_api {
             return Ok(api.clone());
@@ -214,4 +240,64 @@ mod tests {
             c.unwrap().profiles["default"].server_base.as_ref().unwrap()
         );
     }
+
+    #[test]
+    fn config_trims_trailing_forward_slashes_in_urls() {
+        let tmpfile = NamedTempFile::new().unwrap();
+        write!(tmpfile.as_file(), "[profiles.default]").unwrap();
+
+        let _ = update_profile(
+            Some(tmpfile.as_ref()),
+            "default".to_owned(),
+            ProfileKey::server_base,
+            "https://vault.bitwarden.com//////".to_owned(),
+        );
+
+        let _ = update_profile(
+            Some(tmpfile.as_ref()),
+            "default".to_owned(),
+            ProfileKey::server_api,
+            "https://api.bitwarden.com/".to_owned(),
+        );
+
+        let _ = update_profile(
+            Some(tmpfile.as_ref()),
+            "default".to_owned(),
+            ProfileKey::server_identity,
+            "https://identity.bitwarden.com/".to_owned(),
+        );
+
+        let c = load_config(Some(Path::new(tmpfile.as_ref())), true).unwrap();
+        assert_eq!(
+            "https://vault.bitwarden.com",
+            c.profiles["default"].server_base.as_ref().unwrap()
+        );
+        assert_eq!(
+            "https://api.bitwarden.com",
+            c.profiles["default"].server_api.as_ref().unwrap()
+        );
+        assert_eq!(
+            "https://identity.bitwarden.com",
+            c.profiles["default"].server_identity.as_ref().unwrap()
+        );
+    }
+
+    #[test]
+    fn config_does_not_trim_forward_slashes_in_non_url_values() {
+        let tmpfile = NamedTempFile::new().unwrap();
+        write!(tmpfile.as_file(), "[profiles.default]").unwrap();
+
+        let _ = update_profile(
+            Some(tmpfile.as_ref()),
+            "default".to_owned(),
+            ProfileKey::state_dir,
+            "/dev/null/".to_owned(),
+        );
+
+        let c = load_config(Some(Path::new(tmpfile.as_ref())), true).unwrap();
+        assert_eq!(
+            "/dev/null/",
+            c.profiles["default"].state_dir.as_ref().unwrap()
+        );
+    }
 }