copyparty: never send the password to foreign hosts

4 files changed, 165 insertions, 28 deletions

diff --git a/src/commands/utilities.rs b/src/commands/utilities.rs
index b77f44d..33d732f 100644
--- a/src/commands/utilities.rs
+++ b/src/commands/utilities.rs
@@ -4,7 +4,7 @@ use tracing::warn;
 use super::autocomplete::any_file;
 use crate::music::sanitize_yt_url;
 use crate::Data;
-use crate::copyparty::{html_decode, percent_decode};
+use crate::copyparty::{html_decode, percent_decode, redact_url};
 use crate::error::{Error, Result};
 use crate::lang;
 
@@ -502,7 +502,11 @@ async fn run_download(
         let stderr = String::from_utf8_lossy(&output.stderr);
         // Keep the full yt-dlp trace in the logs; the user only sees the
         // short `fmt_download_failed` (translated) message.
-        warn!(url = %url, "yt-dlp failed:\n{}", stderr.trim_end());
+        warn!(
+            url = %redact_url(url),
+            "yt-dlp failed:\n{}",
+            stderr.trim_end()
+        );
         let _ = tokio::fs::remove_dir_all(&dl_dir).await;
         ctx.say(lang::fmt_download_failed(&stderr)).await?;
         return Ok(());
diff --git a/src/copyparty/mod.rs b/src/copyparty/mod.rs
index d3620b2..1f075a0 100644
--- a/src/copyparty/mod.rs
+++ b/src/copyparty/mod.rs
@@ -1,6 +1,6 @@
 use std::time::Duration;
 
-use reqwest::Client;
+use reqwest::{Client, Url};
 use serde::Deserialize;
 use tracing::warn;
 
@@ -100,6 +100,88 @@ fn hex_nibble(b: u8) -> Option<u8> {
     }
 }
 
+/// Replace Copyparty's `pw` query parameter with `***` for logs and errors.
+/// Leaves non-secret URLs untouched.
+pub fn redact_url(url: &str) -> String {
+    let Ok(mut parsed) = Url::parse(url) else {
+        return url.to_string();
+    };
+    let mut redacted = false;
+    let pairs: Vec<(String, String)> = parsed
+        .query_pairs()
+        .map(|(key, value)| {
+            let value = if key.eq_ignore_ascii_case("pw") {
+                redacted = true;
+                "***".to_string()
+            } else {
+                value.into_owned()
+            };
+            (key.into_owned(), value)
+        })
+        .collect();
+    if redacted {
+        parsed.query_pairs_mut().clear();
+        parsed.query_pairs_mut().extend_pairs(pairs);
+    }
+    parsed.to_string()
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn client(password: Option<&str>) -> CopypartyClient {
+        CopypartyClient::new(
+            "http://localhost:3939".into(),
+            "http://localhost:3939/uploads".into(),
+            password.map(str::to_string),
+        )
+    }
+
+    #[test]
+    fn authenticates_only_own_host() {
+        let c = client(Some("s3cret"));
+        assert!(c.authenticate_url("http://localhost:3939/a.mp3").contains("pw=s3cret"));
+        assert_eq!(
+            c.authenticate_url("https://evil.example/a.mp3"),
+            "https://evil.example/a.mp3"
+        );
+        // Same host, different scheme/port is not our Copyparty.
+        assert_eq!(
+            c.authenticate_url("https://localhost:3939/a.mp3"),
+            "https://localhost:3939/a.mp3"
+        );
+    }
+
+    #[test]
+    fn encodes_password_and_keeps_existing_query() {
+        let c = client(Some("p&w/word"));
+        let url = c.authenticate_url("http://localhost:3939/a.mp3?download=1");
+        assert!(url.contains("download=1"), "{url}");
+        assert!(url.contains("pw=p%26w%2Fword"), "{url}");
+        assert_eq!(url.matches("pw=").count(), 1, "{url}");
+    }
+
+    #[test]
+    fn build_url_for_handles_paths_and_urls() {
+        let c = client(Some("pw"));
+        assert!(c.build_url_for("/music/a.mp3").contains("pw=pw"));
+        assert_eq!(
+            c.build_url_for("https://evil.example/a.mp3"),
+            "https://evil.example/a.mp3"
+        );
+    }
+
+    #[test]
+    fn redact_hides_password() {
+        assert_eq!(
+            redact_url("http://localhost:3939/a.mp3?pw=secret&x=1"),
+            "http://localhost:3939/a.mp3?pw=***&x=1"
+        );
+        assert_eq!(redact_url("https://example.com/a"), "https://example.com/a");
+    }
+}
+
 /// A search result from copyparty's `/?srch` API.
 ///
 /// **Important**: `/?srch` responses use `rp` (relative path like `/music/foo.mp3`).
@@ -159,12 +241,7 @@ impl CopypartyClient {
     /// if a password is configured. Path can be absolute (e.g. `/music/foo.mp3`)
     /// or relative (`music/foo.mp3`).
     pub fn build_url(&self, path: &str) -> String {
-        let prefixed = self.build_url_no_pw(path);
-        if let Some(pw) = &self.password {
-            format!("{prefixed}?pw={pw}")
-        } else {
-            prefixed
-        }
+        self.authenticate_url(&self.build_url_no_pw(path))
     }
 
     /// Build full URL without the password parameter.
@@ -176,6 +253,54 @@ impl CopypartyClient {
         }
     }
 
+    /// Add the Copyparty password to a URL **only when it points at our own
+    /// Copyparty host**. This is the safe variant of `build_url` for URLs that
+    /// may have come from a user (cron audio, playlists, redirects): a foreign
+    /// host must never receive `?pw=...`.
+    pub fn authenticate_url(&self, url: &str) -> String {
+        let Some(password) = &self.password else {
+            return url.to_string();
+        };
+        if !self.is_own_url(url) {
+            return url.to_string();
+        }
+        let Ok(mut parsed) = Url::parse(url) else {
+            return url.to_string();
+        };
+        // Replace any existing `pw` parameter rather than appending a second
+        // one (also handles URLs that already carry a query string).
+        let pairs: Vec<(String, String)> = parsed
+            .query_pairs()
+            .filter(|(key, _)| key != "pw")
+            .map(|(key, value)| (key.into_owned(), value.into_owned()))
+            .collect();
+        parsed.query_pairs_mut().clear();
+        parsed.query_pairs_mut().extend_pairs(pairs);
+        parsed.query_pairs_mut().append_pair("pw", password);
+        parsed.to_string()
+    }
+
+    /// Accept either a path or a full URL: paths are resolved against the
+    /// configured Copyparty base URL, absolute URLs are authenticated only
+    /// when they belong to Copyparty.
+    pub fn build_url_for(&self, path_or_url: &str) -> String {
+        if path_or_url.starts_with("http://") || path_or_url.starts_with("https://") {
+            self.authenticate_url(path_or_url)
+        } else {
+            self.build_url(path_or_url)
+        }
+    }
+
+    /// Is `url` the same origin as the configured Copyparty base URL?
+    fn is_own_url(&self, url: &str) -> bool {
+        let (Ok(candidate), Ok(base)) = (Url::parse(url), Url::parse(&self.base_url)) else {
+            return false;
+        };
+        candidate.scheme() == base.scheme()
+            && candidate.host_str() == base.host_str()
+            && candidate.port_or_known_default() == base.port_or_known_default()
+    }
+
     /// Search copyparty via the jPOST `/?srch` endpoint.
     ///
     /// Uses `serde_json::Value` for flexible response parsing (mirroring the Python
diff --git a/src/tasks/mod.rs b/src/tasks/mod.rs
index f0585ef..b492f61 100644
--- a/src/tasks/mod.rs
+++ b/src/tasks/mod.rs
@@ -125,19 +125,15 @@ async fn execute_audio_job(ctx: &CronContext, job: &crate::database::CronJob) {
 
     let channel_id = serenity::ChannelId::new(job.channel_id as u64);
 
-    // Build the full URL with password
-    let url = if audio_url.starts_with("http://") || audio_url.starts_with("https://") {
-        // Already a full URL (stored without password), append it now
-        if let Some(pw) = &ctx.copyparty.password {
-            format!("{audio_url}?pw={pw}")
-        } else {
-            audio_url.clone()
-        }
-    } else {
-        // Legacy relative path — build full URL with password
-        ctx.copyparty.build_url(audio_url)
-    };
-    info!("Cron audio job {} playing: {url}", job.id);
+    // Authenticate only when the URL belongs to our Copyparty host; a cron
+    // job may point at an arbitrary third-party URL and must never receive
+    // the password.
+    let url = ctx.copyparty.build_url_for(audio_url);
+    info!(
+        "Cron audio job {} playing: {}",
+        job.id,
+        crate::copyparty::redact_url(&url)
+    );
     let channel = match channel_id.to_channel(&ctx.http).await {
         Ok(serenity::Channel::Guild(ch)) => ch,
         other => {
@@ -175,8 +171,11 @@ async fn execute_audio_job(ctx: &CronContext, job: &crate::database::CronJob) {
     }
 
     info!(
-        "Cron audio job {} playing '{}' (url: {url}) in guild {}",
-        job.id, audio_url, channel.guild_id
+        "Cron audio job {} playing '{}' (url: {}) in guild {}",
+        job.id,
+        audio_url,
+        crate::copyparty::redact_url(&url),
+        channel.guild_id
     );
 }
 
diff --git a/src/voice/mod.rs b/src/voice/mod.rs
index fcb0b07..5a89f93 100644
--- a/src/voice/mod.rs
+++ b/src/voice/mod.rs
@@ -16,6 +16,7 @@ use symphonia::core::meta::MetadataOptions;
 use tokio::sync::Mutex;
 use tracing::{info, warn};
 
+use crate::copyparty::redact_url;
 use crate::error::{Error, Result};
 use crate::lang;
 use crate::recording::{Recorder, RecorderEventHandler};
@@ -241,7 +242,11 @@ pub async fn ytdlp_download(url: &str) -> Result<Vec<u8>> {
         let stderr = String::from_utf8_lossy(&output.stderr);
         // Keep the full yt-dlp trace in the logs for debugging — the user
         // only ever sees the short, translated `fmt_ytdlp_stderr` message.
-        warn!(url = %url, "yt-dlp failed:\n{}", stderr.trim_end());
+        warn!(
+            url = %redact_url(url),
+            "yt-dlp failed:\n{}",
+            stderr.trim_end()
+        );
         return Err(Error::Custom(lang::fmt_ytdlp_stderr(&stderr)));
     }
 
@@ -256,11 +261,11 @@ pub async fn download_url(client: &reqwest::Client, url: &str) -> Result<Vec<u8>
         .get(url)
         .send()
         .await
-        .map_err(|e| Error::Custom(format!("HTTP GET failed for {url}: {e}")))?;
+        .map_err(|e| Error::Custom(format!("HTTP GET failed for {}: {e}", redact_url(url))))?;
     let bytes = resp
         .bytes()
         .await
-        .map_err(|e| Error::Custom(format!("Failed to read body for {url}: {e}")))?;
+        .map_err(|e| Error::Custom(format!("Failed to read body for {}: {e}", redact_url(url))))?;
     Ok(bytes.to_vec())
 }
 
@@ -398,7 +403,11 @@ pub async fn ffmpeg_stream_input(url: &str) -> Result<songbird::input::Input> {
             let _ = reader.read_to_end(&mut buf).await;
             let text = String::from_utf8_lossy(&buf);
             if !text.is_empty() {
-                warn!(url = %url_owned, "ffmpeg stderr: {}", text.trim_end());
+                warn!(
+                    url = %redact_url(&url_owned),
+                    "ffmpeg stderr: {}",
+                    text.trim_end()
+                );
             }
         });
     }