From 64afa0fd8c863deb6a1568428dfaf2ce67ef9085 Mon Sep 17 00:00:00 2001 From: salmonik Date: Wed, 4 Nov 2026 01:06:38 +0100 Subject: copyparty: never send the password to foreign hosts --- src/commands/utilities.rs | 8 ++- src/copyparty/mod.rs | 139 +++++++++++++++++++++++++++++++++++++++++++--- src/tasks/mod.rs | 29 +++++----- src/voice/mod.rs | 17 ++++-- 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 { } } +/// 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> { 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 .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 { 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() + ); } }); } -- cgit v1.2.3