fix: keep database and redis passwords out of the process output

src/config.rs

@@ -17,13 +17,27 @@ use nextcloud_config_parser::{
 };
 use redis::{ConnectionAddr, ConnectionInfo};
 use sqlx::any::AnyConnectOptions;
+use sqlx::ConnectOptions;
 use std::convert::{TryFrom, TryInto};
 use std::env::var;
 use std::fmt::{Debug, Display, Formatter};
 use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, TcpListener};
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 
+const REDACTED: &str = "REDACTED";
+
+/// Parses a url given on the command line.
+///
+/// Both url options are taken as plain strings so that clap never has to format
+/// a parse error: it puts the raw argument into its own message, password and all.
+fn parse_url_option<T: FromStr>(option: &'static str, url: &str) -> Result<T, ConfigError>
+where
+    T::Err: std::error::Error + Send + Sync + 'static,
+{
+    T::from_str(url).map_err(|e| ConfigError::UrlOption(option, Box::new(e)))
+}
+
 fn styles() -> Styles {
     Styles::styled()
         .header(AnsiColor::Yellow.on_default() | Effects::BOLD)
@@ -32,15 +46,15 @@ fn styles() -> Styles {
         .placeholder(AnsiColor::Green.on_default())
 }
 
-#[derive(Parser, Debug)]
+#[derive(Parser)]
 #[command(name = "notify_push", styles = styles())]
 pub struct Opt {
     /// The database connect url
     #[clap(long)]
-    pub database_url: Option<AnyConnectOptions>,
+    pub database_url: Option<String>,
     /// The redis connect url
     #[clap(long)]
-    pub redis_url: Vec<ConnectionInfo>,
+    pub redis_url: Vec<String>,
     /// The client certificate to use when connecting to redis over TLS
     #[clap(long)]
     pub redis_tls_cert: Option<PathBuf>,
@@ -118,7 +132,6 @@ pub struct Opt {
     pub max_connection_time: Option<usize>,
 }
 
-#[derive(Debug)]
 pub struct Config {
     pub database: AnyConnectOptions,
     pub database_prefix: String,
@@ -135,6 +148,66 @@ pub struct Config {
     pub max_connection_time: usize,
 }
 
+/// Formats a database url without its password
+struct RedactedDatabase<'a>(&'a AnyConnectOptions);
+
+impl Debug for RedactedDatabase<'_> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        let mut url = self.0.to_url_lossy();
+        if url.password().is_some() && url.set_password(Some(REDACTED)).is_err() {
+            // never fall through to printing the url we failed to redact
+            return f.write_str(REDACTED);
+        }
+        Display::fmt(&url, f)
+    }
+}
+
+/// Formats a redis config without its password
+struct RedactedRedis<'a>(&'a RedisConfig);
+
+impl Debug for RedactedRedis<'_> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        match self.0 {
+            RedisConfig::Single(single) => f
+                .debug_struct("Single")
+                .field("addr", &single.addr)
+                .field("db", &single.db)
+                .field("username", &single.username)
+                .field("password", &single.password.as_ref().map(|_| REDACTED))
+                .field("tls_params", &single.tls_params)
+                .finish(),
+            RedisConfig::Cluster(cluster) => f
+                .debug_struct("Cluster")
+                .field("addr", &cluster.addr)
+                .field("db", &cluster.db)
+                .field("username", &cluster.username)
+                .field("password", &cluster.password.as_ref().map(|_| REDACTED))
+                .field("tls_params", &cluster.tls_params)
+                .finish(),
+        }
+    }
+}
+
+impl Debug for Config {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("Config")
+            .field("database", &RedactedDatabase(&self.database))
+            .field("database_prefix", &self.database_prefix)
+            .field("redis", &RedactedRedis(&self.redis))
+            .field("nextcloud_url", &self.nextcloud_url)
+            .field("metrics_bind", &self.metrics_bind)
+            .field("log_level", &self.log_level)
+            .field("bind", &self.bind)
+            .field("allow_self_signed", &self.allow_self_signed)
+            .field("user_agent", &self.user_agent)
+            .field("no_ansi", &self.no_ansi)
+            .field("tls", &self.tls)
+            .field("max_debounce_time", &self.max_debounce_time)
+            .field("max_connection_time", &self.max_connection_time)
+            .finish()
+    }
+}
+
 #[derive(Debug, Clone)]
 pub struct TlsConfig {
     pub key: PathBuf,
@@ -247,13 +320,13 @@ impl Config {
             .transpose()?
             .unwrap_or_default();
         let from_env = PartialConfig::from_env()?;
-        let from_opt = PartialConfig::from_opt(opt);
+        let from_opt = PartialConfig::from_opt(opt)?;
 
         from_opt.merge(from_env).merge(from_config).try_into()
     }
 }
 
-#[derive(Debug, Default)]
+#[derive(Default)]
 struct PartialConfig {
     pub database: Option<AnyConnectOptions>,
     pub database_prefix: Option<String>,
@@ -359,17 +432,23 @@ impl PartialConfig {
         Ok(parse_config_file(file, glob)?)
     }
 
-    fn from_opt(opt: Opt) -> Self {
+    fn from_opt(opt: Opt) -> Result<Self> {
         let tls = if let (Some(cert), Some(key)) = (opt.tls_cert, opt.tls_key) {
             Some(TlsConfig { cert, key })
         } else {
             None
         };
 
-        let redis = match opt.redis_url.len() {
+        let redis_url = opt
+            .redis_url
+            .iter()
+            .map(|url| parse_url_option::<ConnectionInfo>("redis-url", url))
+            .collect::<Result<Vec<_>, _>>()?;
+
+        let redis = match redis_url.len() {
             0 => None,
             1 => {
-                let redis = opt.redis_url.into_iter().next().unwrap();
+                let redis = redis_url.into_iter().next().unwrap();
                 let addr = map_redis_addr(redis.addr().clone());
 
                 let redis_tls = matches!(addr, RedisConnectionAddr::Tcp { tls: true, .. });
@@ -391,8 +470,7 @@ impl PartialConfig {
                 }))
             }
             _ => {
-                let addr: Vec<_> = opt
-                    .redis_url
+                let addr: Vec<_> = redis_url
                     .iter()
                     .map(|redis| map_redis_addr(redis.addr().clone()))
                     .collect();
@@ -410,7 +488,7 @@ impl PartialConfig {
                     insecure: opt.redis_tls_insecure,
                 });
 
-                let redis = opt.redis_url.into_iter().next().unwrap();
+                let redis = redis_url.into_iter().next().unwrap();
                 let redis = redis.redis_settings();
                 Some(RedisConfig::Cluster(RedisClusterConnectionInfo {
                     addr,
@@ -422,8 +500,12 @@ impl PartialConfig {
             }
         };
 
-        PartialConfig {
-            database: opt.database_url,
+        Ok(PartialConfig {
+            database: opt
+                .database_url
+                .as_deref()
+                .map(|url| parse_url_option("database-url", url))
+                .transpose()?,
             database_prefix: opt.database_prefix,
             redis,
             nextcloud_url: opt.nextcloud_url,
@@ -444,7 +526,7 @@ impl PartialConfig {
             tls,
             max_debounce_time: opt.max_debounce_time,
             max_connection_time: opt.max_connection_time,
-        }
+        })
     }
 
     fn merge(self, fallback: Self) -> Self {
@@ -502,3 +584,94 @@ fn map_redis_addr(addr: ConnectionAddr) -> RedisConnectionAddr {
         _ => unreachable!("unknown redis address"),
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn test_config(database: &str, redis_password: Option<&str>) -> Config {
+        sqlx::any::install_default_drivers();
+        Config {
+            database: database.parse().unwrap(),
+            database_prefix: "oc_".into(),
+            redis: RedisConfig::Single(RedisConnectionInfo {
+                addr: RedisConnectionAddr::Tcp {
+                    host: "127.0.0.1".into(),
+                    port: 6379,
+                    tls: false,
+                },
+                db: 0,
+                username: Some("redis_user".into()),
+                password: redis_password.map(String::from),
+                tls_params: None,
+            }),
+            nextcloud_url: "https://cloud.example.com/".into(),
+            metrics_bind: None,
+            log_level: "warn".into(),
+            bind: Bind::Tcp(([127, 0, 0, 1], 7867).into()),
+            allow_self_signed: false,
+            user_agent: None,
+            no_ansi: false,
+            tls: None,
+            max_debounce_time: 15,
+            max_connection_time: 0,
+        }
+    }
+
+    /// Renders an error together with its whole source chain
+    fn full_error(err: &dyn std::error::Error) -> String {
+        let mut rendered = err.to_string();
+        let mut source = err.source();
+        while let Some(inner) = source {
+            rendered.push_str(" / ");
+            rendered.push_str(&inner.to_string());
+            source = inner.source();
+        }
+        rendered
+    }
+
+    #[test]
+    fn test_malformed_urls_are_rejected_without_echoing_them() {
+        let opt = Opt::try_parse_from([
+            "notify_push",
+            "--database-url",
+            "postgres://ncuser:db-hunter2@host:not_a_port/nextcloud",
+            "--redis-url",
+            "redis://redisuser:redis-hunter2@host:not_a_port/0",
+        ])
+        .expect("clap has to accept the raw value and leave validation to us");
+
+        let err = Config::from_opt(opt).expect_err("a malformed url still has to be rejected");
+        let rendered = full_error(&err);
+        assert!(!rendered.contains("db-hunter2"), "{rendered}");
+        assert!(!rendered.contains("redis-hunter2"), "{rendered}");
+    }
+
+    #[test]
+    fn test_config_debug_does_not_leak_passwords() {
+        // `--dump-config` and `log::trace!` both format the whole config
+        let config = test_config(
+            "postgres://nextcloud:db-hunter2@localhost/nextcloud",
+            Some("redis-hunter2"),
+        );
+        for formatted in [format!("{config:?}"), format!("{config:#?}")] {
+            assert!(
+                !formatted.contains("db-hunter2"),
+                "database password leaked into config debug output: {formatted}"
+            );
+            assert!(
+                !formatted.contains("redis-hunter2"),
+                "redis password leaked into config debug output: {formatted}"
+            );
+        }
+    }
+
+    #[test]
+    fn test_config_debug_keeps_non_secret_details() {
+        let config = test_config("postgres://nextcloud@localhost/nextcloud", None);
+        let formatted = format!("{config:#?}");
+        assert!(formatted.contains("localhost"), "{formatted}");
+        assert!(formatted.contains("cloud.example.com"), "{formatted}");
+        assert!(formatted.contains("redis_user"), "{formatted}");
+    }
+}

src/error.rs

@@ -100,6 +100,11 @@ pub enum ConfigError {
     #[error("Error while parsing nextcloud config.php")]
     #[diagnostic(transparent)]
     Parse(#[from] nextcloud_config_parser::Error),
+    #[error("Invalid --{0} value")]
+    UrlOption(
+        &'static str,
+        #[source] Box<dyn std::error::Error + Send + Sync>,
+    ),
     #[error("Invalid {0} environment variable")]
     Env(
         &'static str,