dtourolle/jellytau
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 37 Wiki Activity

fix(downloads): confine download paths to the download root

Browse Source 
Both halves of the path a download writes to arrived from the frontend
unchecked. `start_download` and the queue pump built their target as
`PathBuf::from(target_dir).join(file_path)`, and `mark_download_completed`
stored a frontend-supplied `file_path` on the row verbatim — the same
column that is later read back into `std::fs::remove_file` when a
download is deleted. A correct sanitiser already existed and
`download_item_and_start` used it, but `download_item` is a command in
its own right, so calling it directly routed the guard around.

The guard moves inside. `confine_to_root` folds `..` away lexically and
requires the result to sit inside the storage root, modelled on
`media_server::resolve_path` — the check comes after the join because
`Path::join` drops the base when the joined half is absolute, so an
absolute `file_path` is obeyed rather than folded. `confine_queued_path`
sanitises a queued path per component (so the already-safe name
`download_item_and_start` passes in is not sanitised into a second,
different one) and confines it. Applied in `download_item`, at both join
sites, and to what `mark_download_completed` writes.

Every path the app builds for itself is returned unchanged, including
the absolute ones `download_series`/`download_season` produce from
`${targetDir}/videos`, so no existing row or file on disk is orphaned.
The pump fails an offending row rather than skipping it, because the
pump re-queries and would otherwise not terminate.

Not a live vulnerability: reaching these commands with hostile input
needs script execution in a webview whose CSP is `script-src 'self'`.
This is hardening and consistency.

TRACES: DR-211 | UT-205
This commit is contained in:
dtourolle
2026-08-20 20:01:52 +02:00
parent ae26d5356a
commit da6b039b29
1 changed files with 216 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src-tauri/src/commands/download/mod.rs
+216 -5
View File
@@ -3,7 +3,7 @@
#[cfg(test)]
use crate::utils::lock::MutexSafe;
use log::{debug, error, info, warn};
use std::path::PathBuf;
use std::path::{Component, Path, PathBuf};
use std::sync::{Arc, Mutex};
use tauri::{Manager, State};
@@ -132,6 +132,73 @@ fn sanitize_filename(name: &str) -> String {
.collect()
}
/// The directory every download has to stay inside: the storage root
/// `storage_get_path` hands the frontend, which is the database's parent.
///
/// TRACES: DR-211 | UT-205
fn download_root(db: &DatabaseWrapper) -> Result<PathBuf, String> {
let database = db.0.lock().map_err(|e| e.to_string())?;
database
.path()
.parent()
.map(|p| p.to_path_buf())
.ok_or_else(|| "Database path has no parent directory".to_string())
}
/// Fold `..` out of `candidate` and require what is left to sit inside `root`.
///
/// Lexical rather than `canonicalize`, the same way `media_server::resolve_path`
/// does it: the file usually does not exist yet, so canonicalising would fail on
/// the ordinary case. The check has to come *after* the caller's join, because
/// `Path::join` drops the base when the joined half is absolute — such a path is
/// not folded, it is obeyed, and only the `starts_with` below catches it.
///
/// TRACES: DR-211 | UT-205
fn confine_to_root(root: &Path, candidate: &Path) -> Result<PathBuf, String> {
let mut resolved = PathBuf::new();
for component in candidate.components() {
match component {
Component::ParentDir => {
resolved.pop();
}
Component::CurDir => {}
other => resolved.push(other),
}
}
if resolved.starts_with(root) {
Ok(resolved)
} else {
Err(format!(
"Refusing a download path outside the download directory: {}",
candidate.display()
))
}
}
/// Sanitize a queued download's path and confine it to the download directory.
///
/// Every path the app builds for itself comes back unchanged — files on disk and
/// `downloads` rows point at these exact spellings — and [`sanitize_filename`]
/// is idempotent, so the already-safe name `download_item_and_start` passes in
/// is not sanitized into a second, different one.
///
/// TRACES: DR-211 | UT-205
fn confine_queued_path(root: &Path, file_path: &str) -> Result<String, String> {
let mut sanitized = PathBuf::new();
for component in Path::new(file_path).components() {
match component {
Component::Normal(part) => sanitized.push(sanitize_filename(&part.to_string_lossy())),
// Kept as they are, so `confine_to_root` is the single thing
// deciding whether what they add up to is still inside the root.
other => sanitized.push(other),
}
}
confine_to_root(root, &root.join(&sanitized))?;
Ok(sanitized.to_string_lossy().to_string())
}
/// Request payload for download_item_and_start (bundled to stay within specta's
/// 10-argument command limit).
#[derive(Debug, specta::Type, serde::Deserialize)]
@@ -253,6 +320,18 @@ pub async fn download_item(
album_name,
expected_size,
} = request;
// `start_download` joins this onto the target directory, and `Path::join`
// drops the base when the second half is absolute, so the row itself has to
// be confined — not only the place it is used. `download_item_and_start`
// sanitizes the name it builds, but `download_item` is a command in its own
// right, so that guard was simply routed around by calling this directly.
// TRACES: DR-211 | UT-205
let file_path = {
let root = download_root(&db)?;
confine_queued_path(&root, &file_path)?
};
let db_service = {
let database = db.0.lock().map_err(|e| e.to_string())?;
Arc::new(database.service())
@@ -1286,6 +1365,20 @@ pub async fn mark_download_completed(
bytes_downloaded: i64,
file_path: String,
) -> Result<(), String> {
// Deleting a download reads this straight back into `std::fs::remove_file`,
// so a row must never come to name a file outside the download directory.
// The worker reports the absolute path it wrote, and joining an absolute
// path onto the root yields it unchanged, so that case is stored verbatim;
// the frontend's fallback to the row's own (relative) path resolves under
// the root, where the worker put it.
// TRACES: DR-211 | UT-205
let file_path = {
let root = download_root(&db)?;
confine_to_root(&root, &root.join(&file_path))?
.to_string_lossy()
.to_string()
};
let db_service = {
let database = db.0.lock().map_err(|e| e.to_string())?;
Arc::new(database.service())
@@ -1416,6 +1509,14 @@ pub async fn start_download(
item_id, file_path, file_size
);
// Both halves of this join reached us from the frontend, so resolve them
// against the download directory before a single byte is written.
// TRACES: DR-211 | UT-205
let target_path = {
let root = download_root(&db)?;
confine_to_root(&root, &PathBuf::from(&target_dir).join(&file_path))?
};
// Make a HEAD request to get the file size from Content-Length header
debug!("Making HEAD request to get file size...");
let head_response = reqwest::Client::new().head(&stream_url).send().await;
@@ -1489,9 +1590,6 @@ pub async fn start_download(
Err(e) => error!(" Event emit failed: {:?}", e),
}
// Build target path
let target_path = PathBuf::from(&target_dir).join(&file_path);
// Get a clone of the active downloads Arc for unregistering later
let active_downloads = {
let manager = download_manager.0.lock().map_err(|e| e.to_string())?;
@@ -1762,6 +1860,36 @@ pub(crate) async fn pump_download_queue(
None => return, // Nothing pending to start
};
// Confine the row's path before it takes a slot. A row whose target
// escapes the download directory can never start, so it is failed here
// rather than picked again on the next pass — this loop re-queries, so
// merely skipping it would not terminate.
// TRACES: DR-211 | UT-205
let confined = {
let db_state = app.state::<DatabaseWrapper>();
download_root(&db_state).and_then(|root| {
confine_to_root(&root, &PathBuf::from(&target_dir).join(&file_path))
})
};
let target_path = match confined {
Ok(path) => path,
Err(e) => {
error!("[pump] Refusing download {}: {}", download_id, e);
let fail_query = Query::with_params(
"UPDATE downloads SET status = 'failed', error_message = ? WHERE id = ?",
vec![QueryParam::String(e), QueryParam::Int64(download_id)],
);
if let Err(db_err) = db_service.execute(fail_query).await {
error!(
"[pump] Failed to mark download {} failed: {}",
download_id, db_err
);
return;
}
continue;
}
};
// Register the slot. If registration fails (race: another pump filled
// the last slot), stop — we'll be re-pumped when a slot frees.
{
@@ -1809,7 +1937,6 @@ pub(crate) async fn pump_download_queue(
},
);
let target_path = PathBuf::from(&target_dir).join(&file_path);
spawn_download_worker(
app.clone(),
download_id,
@@ -2421,6 +2548,90 @@ mod tests {
assert_eq!(sanitize_filename("track/1.flac"), "track_1.flac");
}
/// The download directory as it looks on a device, for the path tests.
const TEST_ROOT: &str = "/data/data/com.dtourolle.jellytau/files";
/// A queued `file_path` cannot walk out of the download directory.
///
/// `download_item` is a command in its own right, so sanitizing in
/// `download_item_and_start` was routed around by invoking it directly, and
/// `start_download` then joined the raw string onto the target directory.
///
/// TRACES: DR-211 | UT-205
#[test]
fn test_queued_download_paths_cannot_escape_the_download_directory() {
let root = Path::new(TEST_ROOT);
assert!(confine_queued_path(root, "downloads/../../../../etc/cron.d/pwn").is_err());
assert!(confine_queued_path(root, "../.bashrc").is_err());
assert!(confine_queued_path(root, "/etc/cron.d/pwn").is_err());
// Why the absolute case needs its own guard rather than folding: the
// join the download path performs discards the base entirely.
assert_eq!(
PathBuf::from(root).join("/etc/cron.d/pwn"),
PathBuf::from("/etc/cron.d/pwn")
);
}
/// The paths the app builds for itself have to survive unchanged: files are
/// already on disk and `downloads` rows point at these exact spellings.
///
/// TRACES: DR-211 | UT-205
#[test]
fn test_queued_download_paths_are_otherwise_unchanged() {
let root = Path::new(TEST_ROOT);
for path in [
"downloads/9f8e7d6c", // MediaCard's queue-for-reconnect
"videos/movies/Arrival.mp4", // VideoDownloadButton
"albums/abc123/01 - Opening.mp3", // queue_album_tracks
// download_series/download_season build an absolute path, because
// their base_path is `${targetDir}/videos`.
"/data/data/com.dtourolle.jellytau/files/videos/Show/S01E02_Pilot.mp4",
] {
assert_eq!(confine_queued_path(root, path).unwrap(), path);
}
// `download_item_and_start` sanitizes the name before calling
// `download_item`; sanitizing it again must not yield a second, different
// name, which would orphan the row and the file it names.
let already = format!("downloads/{}.mp3", sanitize_filename("AC/DC: Live?"));
assert_eq!(confine_queued_path(root, &already).unwrap(), already);
}
/// A completed row's `file_path` is read straight back into
/// `std::fs::remove_file` when the download is deleted, so `mark_download_completed`
/// must not be able to register a file outside the download directory.
///
/// TRACES: DR-211 | UT-205
#[test]
fn test_a_completed_download_cannot_register_a_file_outside_the_root() {
let root = Path::new(TEST_ROOT);
// What the worker actually reports — the absolute path it wrote. Stored
// exactly as it arrives.
let written = "/data/data/com.dtourolle.jellytau/files/downloads/9f8e7d6c";
assert_eq!(
confine_to_root(root, Path::new(written)).unwrap(),
PathBuf::from(written)
);
// The row's own path, if the frontend falls back to it: relative, and it
// resolves to where the worker wrote the file.
assert_eq!(
confine_to_root(root, &root.join("downloads/9f8e7d6c")).unwrap(),
PathBuf::from(written)
);
assert!(confine_to_root(root, Path::new("/home/u/.ssh/id_ed25519")).is_err());
assert!(confine_to_root(
root,
Path::new("/data/data/com.dtourolle.jellytau/files/../../../../etc/passwd")
)
.is_err());
}
/// Helper to set up test database with required foreign key data
fn setup_test_db() -> Database {
let db = Database::open_in_memory().unwrap();