From b44b71cfabe9898d5eada9b07cb583fe995ed45e Mon Sep 17 00:00:00 2001 From: fufesou Date: Wed, 21 Oct 2026 11:55:00 +0800 Subject: [PATCH] Fix/download path validation (#16601) * fix: validate download destinations and peer filename rules Reject unsafe or linked download destinations and validate empty-directory paths before creating them. Bind overwrite approval to accepted transfer entries and propagate receiver errors to the UI. Validate remote listings using the peer's filename rules so Unix names remain browsable on Windows. Reject locally unsafe basenames before starting downloads from either the Flutter or legacy UI. Finish the previous file before validating the next digest so rejecting the next destination preserves the file already received. Require confirmation before restarting interrupted downloads with stale staging files. Preserve matching resumes and cover receiver recovery. Retain explicit overwrite rejection for peers older than 1.1.10 that cannot negotiate confirmation. Signed-off-by: fufesou * comments Signed-off-by: fufesou * fix: use async I/O for download preparation and resume Use Tokio filesystem APIs for download setup and resume metadata checks. Run the Windows link-count query on the blocking pool and flush digest writes before returning. Signed-off-by: fufesou --------- Signed-off-by: fufesou --- flutter/lib/models/file_model.dart | 55 +++++ flutter/lib/utils/download_paths.dart | 52 ++++ flutter/test/download_path_test.dart | 174 ++++++++++++++ libs/base/src/fs.rs | 38 ++- libs/base/src/fs/download.rs | 331 ++++++++++++++++++++++++++ libs/base/src/fs/download_tests.rs | 324 +++++++++++++++++++++++++ src/client/io_loop.rs | 54 ++++- 7 files changed, 1014 insertions(+), 14 deletions(-) create mode 100644 flutter/lib/utils/download_paths.dart create mode 100644 flutter/test/download_path_test.dart create mode 100644 libs/base/src/fs/download.rs create mode 100644 libs/base/src/fs/download_tests.rs diff --git a/flutter/lib/models/file_model.dart b/flutter/lib/models/file_model.dart index 26396bce5..afa5c10a7 100644 --- a/flutter/lib/models/file_model.dart +++ b/flutter/lib/models/file_model.dart @@ -4,6 +4,7 @@ import 'dart:convert'; import 'package:flutter/material.dart'; import 'package:flutter_hbb/common.dart'; import 'package:flutter_hbb/common/widgets/dialog.dart'; +import 'package:flutter_hbb/utils/download_paths.dart'; import 'package:flutter_hbb/utils/event_loop.dart'; import 'package:get/get.dart'; import 'package:path/path.dart' as path; @@ -627,6 +628,17 @@ class FileController { final transferJobIds = []; for (var from in items.items) { final jobID = jobController.addTransferJob(from, isRemoteToLocal); + // Keep this check before PathUtil.join: Rust does not receive the selected + // root separately, and Web transfers bypass Rust validation. + if (isRemoteToLocal && + !DownloadPaths.validName(from.name, windows: isWindows)) { + jobController.jobError({ + 'id': '$jobID', + 'file_num': '0', + 'err': 'Invalid file name: ${from.name}', + }); + continue; + } transferJobs.add((from, jobID)); transferJobIds.add(jobID); } @@ -651,6 +663,15 @@ class FileController { return; } + if (isRemoteToLocal) { + for (final (entry, jobID) in transferJobs) { + if (entry.isDirectory) { + await _downloadEmptyDirectories(entry, jobID, otherSideData); + } + } + return; + } + final List entrys = items.items.toList(); var isRemote = isLocal == true ? true : false; @@ -683,6 +704,35 @@ class FileController { }); } + Future _downloadEmptyDirectories( + Entry entry, int jobID, DirectoryData destination) async { + try { + final emptyDirs = await fileFetcher.readEmptyDirs( + entry.path, false, destination.options.showHidden); + final localRoot = PathUtil.join(destination.directory.path, entry.name, + destination.options.isWindows); + final paths = emptyDirs + .map((dir) => DownloadPaths.mapDirectory( + entry.path, dir.path, localRoot, + remoteWindows: options.value.isWindows, + localWindows: destination.options.isWindows)) + .toList(); + for (final path in paths) { + await DownloadPaths.ensureNoLinks(destination.directory.path, path); + } + for (final path in paths) { + await createDirWithRemote(path, false); + } + } on FormatException catch (err) { + await bind.sessionCancelJob(sessionId: sessionId, actId: jobID); + jobController.jobError({ + 'id': '$jobID', + 'file_num': '0', + 'err': err.message, + }); + } + } + bool _removeCheckboxRemember = false; Future removeAction(SelectedItems items) async { @@ -1120,6 +1170,11 @@ class JobController { return true; } final job = jobTable[jobIndex]; + if (job.type == JobType.transfer && + job.isRemoteToLocal && + job.state == JobState.error) { + return true; + } job.recvJobRes = true; if (job.type == JobType.deleteFile) { job.state = JobState.done; diff --git a/flutter/lib/utils/download_paths.dart b/flutter/lib/utils/download_paths.dart new file mode 100644 index 000000000..54d9fe60f --- /dev/null +++ b/flutter/lib/utils/download_paths.dart @@ -0,0 +1,52 @@ +import 'dart:io'; + +import 'package:path/path.dart' as path; + +class DownloadPaths { + static Future ensureNoLinks(String root, String destination) async { + if (!path.isWithin(root, destination)) { + throw FormatException('Invalid download directory: $destination'); + } + var current = root; + for (final name in path.split(path.relative(destination, from: root))) { + current = path.join(current, name); + if (await FileSystemEntity.type(current, followLinks: false) == + FileSystemEntityType.link) { + throw FormatException('Download directory contains a link: $current'); + } + } + } + + static bool validName(String name, {required bool windows}) { + if (name.isEmpty || name == '.' || name == '..' || name.contains('\x00')) { + return false; + } + if (name.contains('/')) return false; + return !windows || + (!name.contains(RegExp(r'[<>:"\\|?*]')) && + !name.endsWith('.') && + !name.endsWith(' ')); + } + + static String mapDirectory( + String remoteRoot, String remotePath, String localRoot, + {required bool remoteWindows, required bool localWindows}) { + final remote = path.Context( + style: remoteWindows ? path.Style.windows : path.Style.posix); + final local = path.Context( + style: localWindows ? path.Style.windows : path.Style.posix); + if (remotePath.contains('\x00') || + remote.split(remotePath).contains('..') || + (!remote.equals(remoteRoot, remotePath) && + !remote.isWithin(remoteRoot, remotePath))) { + throw FormatException('Invalid download directory: $remotePath'); + } + final relative = remote.relative(remotePath, from: remoteRoot); + if (relative == '.') return localRoot; + final names = remote.split(relative); + if (names.any((name) => !validName(name, windows: localWindows))) { + throw FormatException('Invalid download directory: $remotePath'); + } + return local.joinAll([localRoot, ...names]); + } +} diff --git a/flutter/test/download_path_test.dart b/flutter/test/download_path_test.dart new file mode 100644 index 000000000..fca6fe245 --- /dev/null +++ b/flutter/test/download_path_test.dart @@ -0,0 +1,174 @@ +import 'dart:io'; + +import 'package:flutter_hbb/generated_bridge.dart'; +import 'package:flutter_hbb/models/file_model.dart'; +import 'package:flutter_hbb/models/model.dart'; +import 'package:flutter_hbb/models/platform_model.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:uuid/uuid.dart'; + +final _sessionId = UuidValue('00000000-0000-4000-8000-000000000001'); + +class _Binding extends Fake implements RustdeskImpl { + final sent = []; + final created = []; + int nextId = 0; + + @override + String mainGetCommonSync({required String key, dynamic hint}) => + '${++nextId}'; + + @override + int versionToNumber({required String v, dynamic hint}) => + v == '1.3.3' ? 1 : 2; + + @override + Future sessionSendFiles({ + required UuidValue sessionId, + required int actId, + required String path, + required String to, + required int fileNum, + required bool includeHidden, + required bool isRemote, + required bool isDir, + dynamic hint, + }) async { + expect(isRemote, isTrue); + sent.add(to); + } + + @override + Future sessionCreateDir({ + required UuidValue sessionId, + required int actId, + required String path, + required bool isRemote, + dynamic hint, + }) async { + expect(isRemote, isFalse); + created.add(path); + } + + @override + Future sessionCancelJob({ + required UuidValue sessionId, + required int actId, + dynamic hint, + }) async {} +} + +class _Session extends Fake implements FFI { + @override + final String id = 'download-test'; + @override + UuidValue get sessionId => _sessionId; + @override + late final ffiModel = FfiModel(WeakReference(this)); +} + +class _Fetcher extends FileFetcher { + _Fetcher() : super(() => _sessionId); + List emptyDirectories = []; + + @override + Future> readEmptyDirs( + String path, bool isLocal, bool showHidden) async => + emptyDirectories.map((path) => FileDirectory()..path = path).toList(); +} + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + late _Binding binding; + late _Session session; + late _Fetcher fetcher; + late FileController controller; + late DirectoryData destination; + + setUp(() { + binding = _Binding(); + platformFFI.setBindingForTesting(binding); + session = _Session(); + session.ffiModel.pi.version = '1.5.1'; + fetcher = _Fetcher(); + destination = + DirectoryData(FileDirectory()..path = '/downloads', DirectoryOptions()); + controller = FileController( + isLocal: false, + getSessionID: () => _sessionId, + rootState: WeakReference(session), + jobController: JobController(() => _sessionId, () => null), + fileFetcher: fetcher, + getOtherSideDirectoryData: () => destination, + ); + controller.directory.value.path = '/remote'; + }); + + tearDown(() => session.ffiModel.dispose()); + + Future download(String name, {bool directory = false}) async { + final entry = Entry() + ..name = name + ..path = '/remote/$name' + ..entryType = directory ? 0 : 4; + final items = SelectedItems(isLocal: false)..add(entry); + await controller.sendFiles(items, destination); + } + + test('downloads refuse peer listing names that escape the local folder', + () async { + await download('report.txt'); + expect(binding.sent, ['/downloads/report.txt']); + binding.sent.clear(); + for (final windows in [false, true]) { + destination.options.isWindows = windows; + destination.directory.path = windows ? r'C:\downloads' : '/downloads'; + for (final name in [ + '../outside.txt', + '/outside.txt', + '.', + '..', + '', + 'bad\x00name', + if (windows) ...[r'..\outside.txt', r'C:\outside.txt', r'\\host\share'], + ]) { + await download(name); + expect(binding.sent, isEmpty, reason: name); + expect(controller.jobController.jobTable.last.state, JobState.error); + } + } + await controller.jobController.jobDone({ + 'id': '${controller.jobController.jobTable.last.id}', + 'file_num': '0', + }); + expect(controller.jobController.jobTable.last.state, JobState.error); + }); + + test('empty-directory downloads reject paths outside the selected folder', + () async { + for (final path in ['/remote/other/empty', '/remote/docs/../out']) { + fetcher.emptyDirectories = ['/remote/docs/ok', path]; + await download('docs', directory: true); + expect(binding.created, isEmpty, reason: path); + expect(controller.jobController.jobTable.last.state, JobState.error); + } + fetcher.emptyDirectories = ['/remote/docs', '/remote/docs/sub/empty']; + await download('docs', directory: true); + expect(binding.created, ['/downloads/docs', '/downloads/docs/sub/empty']); + }); + + test('empty-directory downloads refuse existing local links', () async { + final temporary = + await Directory.systemTemp.createTemp('rustdesk_download_'); + addTearDown(() => temporary.delete(recursive: true)); + final outside = await Directory('${temporary.path}/outside').create(); + final downloads = await Directory('${temporary.path}/downloads').create(); + await Link('${downloads.path}/docs').create(outside.path); + destination.directory.path = downloads.path; + destination.options.isWindows = Platform.isWindows; + fetcher.emptyDirectories = ['/remote/docs/empty']; + await download('docs', directory: true); + expect(binding.created, isEmpty); + expect(controller.jobController.jobTable.last.state, JobState.error); + }); +} diff --git a/libs/base/src/fs.rs b/libs/base/src/fs.rs index ae481fc44..15fe2e063 100644 --- a/libs/base/src/fs.rs +++ b/libs/base/src/fs.rs @@ -27,6 +27,12 @@ use hbb_common::{ static NEXT_JOB_ID: AtomicI32 = AtomicI32::new(1); +pub mod download; + +#[cfg(test)] +#[path = "fs/download_tests.rs"] +mod download_tests; + pub fn get_next_job_id() -> i32 { NEXT_JOB_ID.fetch_add(1, Ordering::SeqCst) } @@ -417,6 +423,8 @@ pub struct TransferJob { default_overwrite_strategy: Option, #[serde(skip_serializing)] digest: FileDigest, + #[serde(skip)] + download: Option, } #[derive(Debug, Default, Serialize, Deserialize, Clone)] @@ -587,6 +595,7 @@ impl TransferJob { files: Vec::new(), total_size: 0, enable_overwrite_detection, + download: (is_remote && r#type == JobType::Generic).then(Default::default), ..Default::default() } } @@ -648,6 +657,9 @@ impl TransferJob { #[inline] pub fn set_files(&mut self, files: Vec) -> ResultType<()> { + if self.is_download() && !self.files.is_empty() { + bail!("cannot replace an accepted download file list"); + } validate_transfer_file_names(&files)?; if let DataSource::FilePath(base) = &self.data_source { for file in &files { @@ -692,7 +704,12 @@ impl TransferJob { fn resolve_entry_path(&self, base: &PathBuf, name: &str) -> Option { if self.r#type == JobType::Generic { - match join_validated_path(base, name) { + let result = if self.is_download() { + download::validated_target(base, name) + } else { + join_validated_path(base, name) + }; + match result { Ok(path) => Some(path), Err(err) => { log::error!("Invalid file name in transfer job {}: {}", self.id, err); @@ -705,6 +722,12 @@ impl TransferJob { } pub fn modify_time(&self) { + if self.is_download() { + if let Err(err) = self.finish_download() { + log::error!("Failed to finish download {}: {}", self.id, err); + } + return; + } if self.r#type == JobType::Printer { return; } @@ -729,6 +752,9 @@ impl TransferJob { } pub fn remove_download_file(&self) { + if self.is_download() && self.data_stream.is_none() { + return; + } if self.r#type == JobType::Printer { return; } @@ -761,6 +787,9 @@ impl TransferJob { } pub async fn write(&mut self, block: FileTransferBlock) -> ResultType<()> { + if self.is_download() { + return self.write_download(block).await; + } if block.id != self.id { bail!("Wrong id"); } @@ -1160,6 +1189,13 @@ impl TransferJob { } pub async fn confirm(&mut self, r: &FileTransferSendConfirmRequest) -> bool { + if self.is_download() { + self.confirm_download(r); + // Open receiver resume streams in write() so validation errors reach the caller. + if matches!(r.union, Some(file_transfer_send_confirm_request::Union::OffsetBlk(offset)) if offset > 0) { + return true; + } + } if self.file_num() != r.file_num { // This branch will always be hit if: // 1. `confirm()` is called in `ui_cm_interface.rs` diff --git a/libs/base/src/fs/download.rs b/libs/base/src/fs/download.rs new file mode 100644 index 000000000..61bad489f --- /dev/null +++ b/libs/base/src/fs/download.rs @@ -0,0 +1,331 @@ +use super::{ + get_string, is_write_need_confirmation, join_validated_path, validate_file_name_no_traversal, + validate_no_symlink_components, DataSource, DataStream, DigestCheckResult, TransferJob, +}; +use crate::message_proto::{ + file_transfer_send_confirm_request, FileEntry, FileTransferBlock, FileTransferDigest, + FileTransferSendConfirmRequest, FileType, +}; +use hbb_common::{anyhow::anyhow, bail, compress::decompress, ResultType}; +use std::{ + borrow::Cow, + io::ErrorKind, + path::{Path, PathBuf}, +}; +use tokio::{ + fs::{self, OpenOptions}, + io::{AsyncSeekExt, AsyncWriteExt}, +}; + +const SINGLE_LINK_COUNT: u64 = 1; + +#[derive(Default, Debug)] +pub(super) struct DownloadState { + pub(super) confirmation: Option<(i32, u32)>, +} + +pub fn validate_listing(entries: &[FileEntry], peer_windows: bool) -> ResultType<()> { + for entry in entries { + let name = &entry.name; + if entry.entry_type.enum_value() == Ok(FileType::DirDrive) + && matches!(name.as_bytes(), [drive, b':'] if drive.is_ascii_alphabetic()) + { + continue; + } + if name.is_empty() + || name == "." + || name == ".." + || name.contains('\0') + || name.contains('/') + || (peer_windows + && (name.contains('\\') + || matches!(name.as_bytes(), [drive, b':', ..] if drive.is_ascii_alphabetic()))) + { + bail!("invalid file name in remote directory listing: {name}"); + } + } + Ok(()) +} + +pub fn validate_download_name(path: &str, peer_windows: bool) -> ResultType<()> { + let separator = |c| c == '/' || (peer_windows && c == '\\'); + let path = path.trim_end_matches(separator); + validate_component(path.rsplit(separator).next().unwrap_or("")) +} + +fn validate_component(name: &str) -> ResultType<()> { + validate_file_name_no_traversal(name)?; + if name.is_empty() || name == "." || name.contains('/') { + bail!("invalid download file name: {name}"); + } + #[cfg(windows)] + if name.contains(['\\', ':', '<', '>', '"', '|', '?', '*']) + || name.ends_with('.') + || name.ends_with(' ') + { + bail!("invalid Windows download file name: {name}"); + } + Ok(()) +} + +pub(super) fn validated_target(base: &PathBuf, name: &str) -> ResultType { + let parent = base + .parent() + .ok_or_else(|| anyhow!("download destination has no parent"))?; + let leaf = base + .file_name() + .and_then(|name| name.to_str()) + .ok_or_else(|| anyhow!("invalid download destination"))?; + validate_component(leaf)?; + #[cfg(windows)] + for component in name.split(['/', '\\']).filter(|part| !part.is_empty()) { + validate_component(component)?; + } + validate_no_symlink_components(&parent.to_path_buf(), leaf)?; + let target = join_validated_path(base, name)?; + let parent = target + .parent() + .ok_or_else(|| anyhow!("download file has no parent"))?; + let leaf = target + .file_name() + .and_then(|name| name.to_str()) + .ok_or_else(|| anyhow!("invalid download file name"))?; + for suffix in ["download", "digest"] { + validate_no_symlink_components(&parent.to_path_buf(), &format!("{leaf}.{suffix}"))?; + } + Ok(target) +} + +fn remove_if_exists(path: &Path) -> ResultType<()> { + match std::fs::remove_file(path) { + Ok(()) => Ok(()), + Err(err) if err.kind() == ErrorKind::NotFound => Ok(()), + Err(err) => Err(err.into()), + } +} + +async fn remove_if_exists_async(path: &Path) -> ResultType<()> { + match fs::remove_file(path).await { + Ok(()) => Ok(()), + Err(err) if err.kind() == ErrorKind::NotFound => Ok(()), + Err(err) => Err(err.into()), + } +} + +async fn validate_resume_file(file: &tokio::fs::File) -> ResultType<()> { + let metadata = file.metadata().await?; + if !metadata.is_file() { + bail!("download resume staging path is not a regular file"); + } + #[cfg(unix)] + let links = std::os::unix::fs::MetadataExt::nlink(&metadata); + #[cfg(windows)] + let links = { + use std::{mem::MaybeUninit, os::windows::io::AsRawHandle}; + use winapi::{ + shared::minwindef::FALSE, + um::fileapi::{GetFileInformationByHandle, BY_HANDLE_FILE_INFORMATION}, + }; + let file = file.try_clone().await?; + tokio::task::spawn_blocking(move || -> ResultType { + let mut information = MaybeUninit::::uninit(); + if unsafe { + GetFileInformationByHandle(file.as_raw_handle() as _, information.as_mut_ptr()) + } == FALSE + { + return Err(std::io::Error::last_os_error().into()); + } + Ok(u64::from( + unsafe { information.assume_init() }.nNumberOfLinks, + )) + }) + .await?? + }; + if links != SINGLE_LINK_COUNT { + bail!("hard-linked download resume staging file is not allowed"); + } + Ok(()) +} + +impl TransferJob { + pub(super) fn is_download(&self) -> bool { + self.download.is_some() + } + + pub fn download_target(&self, file_num: i32) -> ResultType { + let entry = self + .files + .get(file_num as usize) + .ok_or_else(|| anyhow!("Wrong file number"))?; + let DataSource::FilePath(base) = &self.data_source else { + bail!("download destination is not a file path"); + }; + validated_target(base, &entry.name) + } + + pub fn download_digest( + &mut self, + can_resume: bool, + digest: &FileTransferDigest, + ) -> ResultType { + if self.file_num != digest.file_num && self.data_stream.is_some() { + let result = self.finish_download(); + self.data_stream.take(); + result?; + } + let target = self.download_target(digest.file_num)?; + let resume = can_resume && self.is_resume && !target.try_exists()?; + let result = is_write_need_confirmation(resume, &get_string(&target), digest)?; + if matches!(result, DigestCheckResult::NoSuchFile) + && (Path::new(&format!("{}.download", target.display())).try_exists()? + || Path::new(&format!("{}.digest", target.display())).try_exists()?) + { + return Ok(DigestCheckResult::NeedConfirm(FileTransferDigest { + id: digest.id, + file_num: digest.file_num, + ..Default::default() + })); + } + Ok(result) + } + + pub(super) fn confirm_download(&mut self, request: &FileTransferSendConfirmRequest) { + if request.id != self.id { + return; + } + let Some(download) = self.download.as_mut() else { + return; + }; + download.confirmation = self.files.get(request.file_num as usize).and_then(|_| { + let offset = match request.union { + Some(file_transfer_send_confirm_request::Union::OffsetBlk(offset)) => offset, + Some(file_transfer_send_confirm_request::Union::Skip(false)) => 0, + _ => return None, + }; + Some((request.file_num, offset)) + }); + } + + fn download_confirmation_offset(&self, file_num: i32) -> Option { + let (confirmed, offset) = self.download.as_ref()?.confirmation.as_ref()?; + (*confirmed == file_num).then_some(*offset) + } + + async fn start_download_file(&mut self, file_num: i32) -> ResultType<()> { + let result = self.finish_download(); + self.data_stream.take(); + result?; + let target = self.download_target(file_num)?; + let staging = PathBuf::from(format!("{}.download", target.display())); + let digest = PathBuf::from(format!("{}.digest", target.display())); + let confirmation = self.download_confirmation_offset(file_num); + if (fs::try_exists(&target).await? + || fs::try_exists(&staging).await? + || fs::try_exists(&digest).await?) + && confirmation.is_none() + { + bail!("download overwrite requires confirmation"); + } + if let Some(offset) = confirmation.filter(|offset| *offset > 0) { + return self.resume_download(file_num, offset).await; + } + if let Some(parent) = target.parent() { + fs::create_dir_all(parent).await?; + } + remove_if_exists_async(&staging).await?; + let file = OpenOptions::new() + .write(true) + .create_new(true) + .open(&staging) + .await?; + self.file_num = file_num; + self.data_stream = Some(DataStream::FileStream(file)); + remove_if_exists_async(&digest).await?; + let mut metadata = OpenOptions::new() + .write(true) + .create_new(true) + .open(&digest) + .await?; + metadata + .write_all(serde_json::to_string(&self.digest)?.as_bytes()) + .await?; + metadata.flush().await?; + Ok(()) + } + + async fn resume_download(&mut self, file_num: i32, offset: u32) -> ResultType<()> { + let target = self.download_target(file_num)?; + if fs::try_exists(&target).await? { + bail!("download overwrite requires confirmation"); + } + let digest = fs::metadata(format!("{}.digest", target.display())).await?; + if !digest.is_file() { + bail!("download resume metadata is not a regular file"); + } + let mut file = OpenOptions::new() + .write(true) + .open(format!("{}.download", target.display())) + .await?; + validate_resume_file(&file).await?; + if file.metadata().await?.len() != u64::from(offset) { + bail!("download resume offset does not match staging file size"); + } + file.seek(std::io::SeekFrom::Start(u64::from(offset))) + .await?; + self.file_num = file_num; + self.data_stream = Some(DataStream::FileStream(file)); + self.transferred += u64::from(offset); + self.finished_size += u64::from(offset); + Ok(()) + } + + pub(super) async fn write_download(&mut self, block: FileTransferBlock) -> ResultType<()> { + if block.id != self.id { + bail!("Wrong id"); + } + if block.file_num as usize >= self.files.len() { + bail!("Wrong file number"); + } + if block.file_num != self.file_num || self.data_stream.is_none() { + self.start_download_file(block.file_num).await?; + } + let data = if block.compressed { + Cow::Owned(decompress(&block.data)) + } else { + Cow::Borrowed(block.data.as_ref()) + }; + let Some(DataStream::FileStream(stream)) = self.data_stream.as_mut() else { + bail!("download file stream is missing"); + }; + stream.write_all(&data).await?; + stream.flush().await?; + self.finished_size += data.len() as u64; + self.transferred += block.data.len() as u64; + Ok(()) + } + + pub fn finish_download(&self) -> ResultType<()> { + if !self.is_download() { + self.modify_time(); + return Ok(()); + } + if self.data_stream.is_none() { + return Ok(()); + } + let target = self.download_target(self.file_num)?; + if target.try_exists()? && self.download_confirmation_offset(self.file_num) != Some(0) { + bail!("download overwrite requires confirmation"); + } + std::fs::rename(format!("{}.download", target.display()), &target)?; + remove_if_exists(&PathBuf::from(format!("{}.digest", target.display())))?; + let entry = self + .files + .get(self.file_num as usize) + .ok_or_else(|| anyhow!("Wrong file number"))?; + filetime::set_file_mtime( + &target, + filetime::FileTime::from_unix_time(entry.modified_time as _, 0), + )?; + Ok(()) + } +} diff --git a/libs/base/src/fs/download_tests.rs b/libs/base/src/fs/download_tests.rs new file mode 100644 index 000000000..3c43ee868 --- /dev/null +++ b/libs/base/src/fs/download_tests.rs @@ -0,0 +1,324 @@ +use super::*; + +struct DownloadDir(PathBuf); + +impl DownloadDir { + fn new() -> Self { + static NEXT_DIRECTORY: AtomicI32 = AtomicI32::new(1); + let sequence = NEXT_DIRECTORY.fetch_add(1, Ordering::Relaxed); + let nonce = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let path = std::env::temp_dir().join(format!( + "rustdesk_download_{}_{nonce}_{sequence}", + std::process::id() + )); + std::fs::create_dir(&path).unwrap(); + Self(path) + } + + fn job(&self) -> TransferJob { + self.job_with_files(vec![FileEntry::default()]) + } + + fn job_with_files(&self, files: Vec) -> TransferJob { + TransferJob::new_write( + 1, + JobType::Generic, + "/remote/file.txt".into(), + DataSource::FilePath(self.0.join("file.txt")), + 0, + false, + true, + true, + ) + .with_files(files) + .unwrap() + } +} + +impl Drop for DownloadDir { + fn drop(&mut self) { + std::fs::remove_dir_all(&self.0).unwrap(); + } +} + +fn block() -> FileTransferBlock { + FileTransferBlock { + id: 1, + file_num: 0, + data: b"received".to_vec().into(), + ..Default::default() + } +} + +fn approval(file_num: i32, offset: u32) -> FileTransferSendConfirmRequest { + FileTransferSendConfirmRequest { + id: 1, + file_num, + union: Some(file_transfer_send_confirm_request::Union::OffsetBlk(offset)), + ..Default::default() + } +} + +#[test] +fn download_listing_rejects_unsafe_names_and_keeps_drive_rows() { + for name in ["../outside", "/outside", ".", "..", "", "dir/file"] { + let entry = FileEntry { + name: name.into(), + ..Default::default() + }; + assert!( + download::validate_listing(&[entry], false).is_err(), + "{}", + name + ); + } + let entries = [ + FileEntry { + name: "report.txt".into(), + ..Default::default() + }, + FileEntry { + name: "report:notes".into(), + ..Default::default() + }, + FileEntry { + name: "C:".into(), + entry_type: FileType::DirDrive.into(), + ..Default::default() + }, + ]; + download::validate_listing(&entries, true).unwrap(); +} + +#[tokio::test] +async fn download_requires_confirmation_to_overwrite() { + let dir = DownloadDir::new(); + let target = dir.0.join("file.txt"); + std::fs::write(&target, b"original").unwrap(); + let mut job = dir.job(); + assert!(job.write(block()).await.is_err()); + assert_eq!(std::fs::read(&target).unwrap(), b"original"); + assert!(!dir.0.join("file.txt.download").exists()); + + job.confirm(&approval(0, 0)).await; + job.write(block()).await.unwrap(); + assert!(job.set_files(vec![FileEntry::default()]).is_err()); + job.finish_download().unwrap(); + assert_eq!(std::fs::read(&target).unwrap(), b"received"); +} + +#[tokio::test] +async fn download_requires_separate_confirmation_for_each_existing_file() { + let dir = DownloadDir::new(); + let mut job = dir.job_with_files(vec![ + FileEntry { + name: "first.txt".into(), + ..Default::default() + }, + FileEntry { + name: "second.txt".into(), + ..Default::default() + }, + ]); + job.write(block()).await.unwrap(); + let second = dir.0.join("file.txt/second.txt"); + std::fs::write(&second, b"original").unwrap(); + assert!(job + .write(FileTransferBlock { + file_num: 1, + ..block() + }) + .await + .is_err()); + assert_eq!(std::fs::read(&second).unwrap(), b"original"); + assert_eq!( + std::fs::read(dir.0.join("file.txt/first.txt")).unwrap(), + b"received" + ); + job.confirm(&approval(1, 0)).await; + job.write(FileTransferBlock { + file_num: 1, + ..block() + }) + .await + .unwrap(); + job.finish_download().unwrap(); + assert_eq!(std::fs::read(&second).unwrap(), b"received"); +} + +#[tokio::test] +async fn download_preserves_received_files_when_publication_fails() { + for next_is_digest in [false, true] { + let dir = DownloadDir::new(); + let files = ["first.txt", "second.txt"] + .iter() + .map(|&name| FileEntry { + name: name.into(), + ..Default::default() + }) + .collect(); + let mut job = dir.job_with_files(files); + job.write(block()).await.unwrap(); + let target = dir.0.join("file.txt/first.txt"); + std::fs::write(&target, b"existing").unwrap(); + if next_is_digest { + assert!(job + .download_digest( + true, + &FileTransferDigest { + id: 1, + file_num: 1, + ..Default::default() + }, + ) + .is_err()); + } else { + assert!(job + .write(FileTransferBlock { + file_num: 1, + ..block() + }) + .await + .is_err()); + } + job.remove_download_file(); + assert_eq!(std::fs::read(target).unwrap(), b"existing"); + assert_eq!( + std::fs::read(dir.0.join("file.txt/first.txt.download")).unwrap(), + b"received" + ); + assert!(dir.0.join("file.txt/first.txt.digest").exists()); + } +} + +#[tokio::test] +async fn download_recovers_interrupted_transfer() { + for (resuming, changed) in [(false, false), (true, true), (true, false)] { + let dir = DownloadDir::new(); + let mut digest = FileTransferDigest { + id: 1, + file_size: block().data.len() as _, + last_modified: 1, + ..Default::default() + }; + let mut interrupted = dir.job(); + interrupted.set_digest(digest.file_size, digest.last_modified); + let partial = FileTransferBlock { + data: b"rec".to_vec().into(), + ..block() + }; + interrupted.write(partial).await.unwrap(); + drop(interrupted); + + let mut job = dir.job(); + job.is_resume = resuming; + digest.last_modified += u64::from(changed); + job.set_digest(digest.file_size, digest.last_modified); + let confirmation = match job.download_digest(true, &digest).unwrap() { + DigestCheckResult::NeedConfirm(confirmation) => confirmation, + _ => panic!("interrupted download must require confirmation"), + }; + let offset = if resuming && !changed { + b"rec".len() + } else { + 0 + }; + assert_eq!(confirmation.is_identical, offset > 0); + assert_eq!(confirmation.transferred_size, offset as u64); + assert!(job.write(block()).await.is_err()); + assert_eq!( + std::fs::read(dir.0.join("file.txt.download")).unwrap(), + b"rec" + ); + job.confirm(&approval(0, offset as _)).await; + job.write(FileTransferBlock { + data: block().data.slice(offset..), + ..block() + }) + .await + .unwrap(); + job.finish_download().unwrap(); + assert_eq!(std::fs::read(dir.0.join("file.txt")).unwrap(), b"received"); + } +} + +#[tokio::test] +async fn download_does_not_publish_or_remove_unreceived_staging_files() { + let dir = DownloadDir::new(); + let target = dir.0.join("file.txt"); + let staging = dir.0.join("file.txt.download"); + std::fs::write(&target, b"original").unwrap(); + std::fs::write(&staging, b"another transfer").unwrap(); + let job = dir.job(); + job.modify_time(); + job.remove_download_file(); + assert_eq!(std::fs::read(&target).unwrap(), b"original"); + assert_eq!(std::fs::read(&staging).unwrap(), b"another transfer"); + + let metadata_only = DownloadDir::new(); + let metadata = metadata_only.0.join("file.txt.digest"); + std::fs::write(&metadata, b"unrelated file").unwrap(); + assert!(metadata_only.job().write(block()).await.is_err()); + assert_eq!(std::fs::read(metadata).unwrap(), b"unrelated file"); +} + +#[test] +#[cfg(windows)] +fn download_rejects_windows_streams_and_ambiguous_components() { + let dir = DownloadDir::new(); + for name in ["nested/file:stream", "nested/.. /outside", "nested/file."] { + assert!( + download::validated_target(&dir.0.join("docs"), name).is_err(), + "{}", + name + ); + } +} + +#[tokio::test] +#[cfg_attr(windows, ignore = "requires symlink privilege")] +async fn download_refuses_symlink_destination_directory() { + let dir = DownloadDir::new(); + let outside = dir.0.join("outside"); + let link = dir.0.join("file.txt"); + std::fs::create_dir_all(&outside).unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(&outside, &link).unwrap(); + #[cfg(windows)] + std::os::windows::fs::symlink_dir(&outside, &link).unwrap(); + let mut job = dir.job_with_files(vec![FileEntry { + name: "child.txt".into(), + ..Default::default() + }]); + assert!(job.write(block()).await.is_err()); + job.modify_time(); + assert!(!outside.join("child.txt").exists()); + assert!(!outside.join("child.txt.download").exists()); +} + +#[tokio::test] +#[cfg_attr(windows, ignore = "requires symlink privilege")] +async fn download_refuses_linked_target_and_staging_files() { + for suffix in ["", ".download", ".digest"] { + let dir = DownloadDir::new(); + let outside = dir.0.join("outside.txt"); + let link = dir.0.join(format!("file.txt{suffix}")); + std::fs::write(&outside, b"original").unwrap(); + #[cfg(unix)] + std::os::unix::fs::symlink(&outside, &link).unwrap(); + #[cfg(windows)] + std::os::windows::fs::symlink_file(&outside, &link).unwrap(); + let mut job = dir.job(); + assert!(job.write(block()).await.is_err()); + job.modify_time(); + job.remove_download_file(); + assert_eq!(std::fs::read(&outside).unwrap(), b"original"); + assert!(std::fs::symlink_metadata(link) + .unwrap() + .file_type() + .is_symlink()); + } +} diff --git a/src/client/io_loop.rs b/src/client/io_loop.rs index f9e6e2952..517628987 100644 --- a/src/client/io_loop.rs +++ b/src/client/io_loop.rs @@ -809,7 +809,16 @@ impl Remote { if is_remote { log::debug!("New job {}, write to {} from remote {}", id, to, path); let to = match r#type { - fs::JobType::Generic => fs::DataSource::FilePath(PathBuf::from(&to)), + fs::JobType::Generic => { + if let Err(err) = fs::download::validate_download_name( + &path, + self.handler.peer_platform() == "Windows", + ) { + self.handle_job_status(id, -1, Some(err.to_string())); + return true; + } + fs::DataSource::FilePath(PathBuf::from(&to)) + } fs::JobType::Printer => { fs::DataSource::MemoryCursor(std::io::Cursor::new(Vec::new())) } @@ -1782,6 +1791,15 @@ impl Remote { fs::transform_windows_path(&mut entries); } } + if fd.id <= 0 { + if let Err(err) = fs::download::validate_listing( + &entries, + self.handler.peer_platform() == "Windows", + ) { + self.handle_job_status(fd.id, -1, Some(err.to_string())); + return true; + } + } // We cannot call cancel_transfer_job/handle_job_status while holding // a mutable borrow from fs::get_job(&mut self.write_jobs), so defer // the error handling until after the borrow scope ends. @@ -1877,11 +1895,7 @@ impl Remote { crate::is_support_file_transfer_resume_num( peer_ver, ); - match fs::is_write_need_confirmation( - is_support_resume && job.is_resume, - &write_path, - &digest, - ) { + match job.download_digest(is_support_resume, &digest) { Ok(res) => match res { DigestCheckResult::IsSame => { let req = FileTransferSendConfirmRequest { @@ -1938,13 +1952,19 @@ impl Remote { union: Some(file_transfer_send_confirm_request::Union::OffsetBlk(0)), ..Default::default() }; - job.confirm(&req).await; + // This did not approve replacing an existing local file. let msg = new_send_confirm(req); allow_err!(peer.send(&msg).await); } }, Err(err) => { - println!("error receiving digest: {}", err); + self.cancel_transfer_job(digest.id, peer).await; + self.handle_job_status( + digest.id, + -1, + Some(err.to_string()), + ); + return true; } } } @@ -1953,22 +1973,30 @@ impl Remote { } } Some(file_response::Union::Block(block)) => { + let id = block.id; + let mut write_error = None; if let Some(job) = fs::get_job(block.id, &mut self.write_jobs) { - if let Err(_err) = job.write(block).await { - // to-do: add "skip" for writing job - } + let result = job.write(block).await; if job.r#type == fs::JobType::Generic { + write_error = result.err().map(|err| err.to_string()); self.update_jobs_status(); } } + if let Some(err) = write_error { + self.cancel_transfer_job(id, peer).await; + self.handle_job_status(id, -1, Some(err)); + } } Some(file_response::Union::Done(d)) => { let mut err: Option = None; let mut job_type = fs::JobType::Generic; let mut printer_data = None; if let Some(job) = fs::remove_job(d.id, &mut self.write_jobs) { - job.modify_time(); - err = job.job_error(); + err = job + .finish_download() + .err() + .map(|err| err.to_string()) + .or_else(|| job.job_error()); job_type = job.r#type; printer_data = match job.get_buf_data().await { Ok(d) => d,