fix(downloads): clean up pre-admission replacements

- Authorize exact fingerprint-matched cleanup for staged replacements
- Revalidate lifecycle, ownership, paths, and identity changes
- Add race and regression coverage
This commit is contained in:
NimBold
2026-08-24 09:05:26 +03:30
parent b56ec0a3a8
commit 9a7894a3e2
5 changed files with 381 additions and 25 deletions
+321 -25
View File
@@ -6235,33 +6235,40 @@ async fn remove_download(
let requested_asset_cleanup = delete_assets && !preserve_assets; let requested_asset_cleanup = delete_assets && !preserve_assets;
let ownership_is_missing = owned_paths.is_empty() && primary_path.is_none(); let ownership_is_missing = owned_paths.is_empty() && primary_path.is_none();
let unowned_unadmitted_cleanup_is_noop = if requested_asset_cleanup let (unowned_unadmitted_cleanup_is_noop, unowned_replacement_target) =
&& ownership_is_missing if requested_asset_cleanup && ownership_is_missing && torrent_removal_paths.is_empty() {
&& torrent_removal_paths.is_empty() let persisted =
{ load_persisted_download_item(&app_handle.state::<crate::db::DbState>(), &id)?;
let persisted = load_persisted_download_item( let replacement_target = inspect_unowned_replacement_target(
&app_handle.state::<crate::db::DbState>(), &app_handle,
&id, &persisted,
)?; native_lifecycle_was_admitted,
let expected_assets_present = expected_unadmitted_download_assets_present( )?;
&app_handle, let expected_assets_present =
&persisted, expected_unadmitted_download_assets_present(&app_handle, &persisted)?;
)?; (
should_skip_unowned_asset_cleanup( should_skip_unowned_asset_cleanup(
persisted.status, persisted.status,
persisted.has_been_dispatched.unwrap_or(false), persisted.has_been_dispatched.unwrap_or(false),
native_lifecycle_was_admitted, native_lifecycle_was_admitted,
expected_assets_present, expected_assets_present,
) ),
} else { replacement_target,
false )
}; } else {
let should_delete_assets = requested_asset_cleanup && !unowned_unadmitted_cleanup_is_noop; (false, None)
};
let should_delete_assets = requested_asset_cleanup
&& (!unowned_unadmitted_cleanup_is_noop || unowned_replacement_target.is_some());
if requested_asset_cleanup { if requested_asset_cleanup {
if ownership_is_missing && !unowned_unadmitted_cleanup_is_noop { if ownership_is_missing
&& !unowned_unadmitted_cleanup_is_noop
&& unowned_replacement_target.is_none()
{
return Err( return Err(
"Cannot remove download files because Firelink ownership is unavailable".to_string(), "Cannot remove download files because Firelink ownership is unavailable"
.to_string(),
); );
} }
@@ -6306,6 +6313,9 @@ async fn remove_download(
{ {
cleanup_targets.push(primary.clone()); cleanup_targets.push(primary.clone());
} }
if let Some(replacement) = unowned_replacement_target.as_ref() {
cleanup_targets.push(replacement.target.clone());
}
cleanup_targets.sort_by_key(|path| crate::platform::path_identity(path)); cleanup_targets.sort_by_key(|path| crate::platform::path_identity(path));
cleanup_targets.dedup_by(|left, right| crate::platform::paths_equal(left, right)); cleanup_targets.dedup_by(|left, right| crate::platform::paths_equal(left, right));
for target in cleanup_targets { for target in cleanup_targets {
@@ -6332,6 +6342,15 @@ async fn remove_download(
remove_download_container_assets(primary, &app_handle).await?; remove_download_container_assets(primary, &app_handle).await?;
} }
} }
if let Some(replacement) = unowned_replacement_target.as_ref() {
remove_unowned_replacement_target(
&replacement.target,
&replacement.fingerprint,
permanent_asset_removal,
&app_handle,
)
.await?;
}
} }
if permanent_asset_removal { if permanent_asset_removal {
remove_managed_torrent_permanently(&app_handle, &id).await?; remove_managed_torrent_permanently(&app_handle, &id).await?;
@@ -6386,6 +6405,188 @@ fn should_skip_unowned_asset_cleanup(
&& !expected_assets_present && !expected_assets_present
} }
#[derive(Debug, Clone)]
struct UnownedReplacementTarget {
target: std::path::PathBuf,
fingerprint: String,
}
fn validate_unowned_replacement_target<R: tauri::Runtime>(
app_handle: &tauri::AppHandle<R>,
target: &std::path::Path,
expected_fingerprint: &str,
metadata: &std::fs::Metadata,
) -> Result<(), String> {
if metadata_is_link_or_reparse(metadata) || path_has_symlink_component(target) {
return Err(format!(
"Cannot remove replacement target '{}' because it is a symbolic link",
target.display()
));
}
if !metadata.is_file() {
return Err(format!(
"Cannot remove replacement target '{}' because it is not a regular file",
target.display()
));
}
if !is_safe_path(target, app_handle) {
return Err(format!(
"Download replacement target '{}' is outside an approved location",
target.display()
));
}
if let Some(owner) = crate::download_ownership::owner_for_path(app_handle, target)? {
return Err(format!(
"Cannot remove replacement target because it is owned by Firelink download {owner}"
));
}
if target_fingerprint(target, metadata) != expected_fingerprint {
return Err(
"The replacement target changed after duplicate resolution. Reopen the conflict and choose Replace again."
.to_string(),
);
}
Ok(())
}
fn inspect_unowned_replacement_target<R: tauri::Runtime>(
app_handle: &tauri::AppHandle<R>,
persisted: &crate::ipc::DownloadItem,
native_lifecycle_was_admitted: bool,
) -> Result<Option<UnownedReplacementTarget>, String> {
if native_lifecycle_was_admitted
|| persisted.has_been_dispatched != Some(false)
|| !matches!(
persisted.status,
crate::ipc::DownloadStatus::Ready | crate::ipc::DownloadStatus::Staged
)
|| persisted.is_torrent == Some(true)
{
return Ok(None);
}
let Some(expected_fingerprint) = persisted
.replace_existing_fingerprint
.as_deref()
.filter(|fingerprint| !fingerprint.is_empty())
else {
return Ok(None);
};
let Some(destination) = persisted
.destination
.as_deref()
.map(str::trim)
.filter(|destination| !destination.is_empty())
else {
return Ok(None);
};
let target = crate::download_ownership::expected_primary_path(
app_handle,
destination,
&persisted.file_name,
)?;
let metadata = match std::fs::symlink_metadata(&target) {
Ok(metadata) => metadata,
Err(error) if error.kind() == std::io::ErrorKind::NotFound => {
// A missing target is safe to treat as an idempotent cleanup, but
// an ownership record for that exact path still blocks the
// unowned replacement exception.
if let Some(owner) = crate::download_ownership::owner_for_path(app_handle, &target)? {
return Err(format!(
"Cannot remove replacement target because it is owned by Firelink download {owner}"
));
}
return Ok(None);
}
Err(error) => return Err(format!("Could not inspect replacement target: {error}")),
};
validate_unowned_replacement_target(app_handle, &target, expected_fingerprint, &metadata)?;
Ok(Some(UnownedReplacementTarget {
target,
fingerprint: expected_fingerprint.to_string(),
}))
}
async fn remove_unowned_replacement_target<R: tauri::Runtime>(
target: &std::path::Path,
expected_fingerprint: &str,
permanent: bool,
app_handle: &tauri::AppHandle<R>,
) -> Result<(), String> {
for attempt in 0..=5 {
let metadata = match tokio::fs::symlink_metadata(target).await {
Ok(metadata) => metadata,
Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()),
Err(error) => {
return Err(format!(
"Could not inspect replacement target '{}': {error}",
target.display()
));
}
};
validate_unowned_replacement_target(app_handle, target, expected_fingerprint, &metadata)?;
if permanent {
match tokio::fs::remove_file(target).await {
Ok(()) => return Ok(()),
Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()),
Err(error) if attempt == 5 => {
return Err(format!(
"Could not permanently remove replacement target '{}' after retries: {error}",
target.display()
));
}
Err(_) => {}
}
} else {
let remove_result = match trash::delete(target) {
Ok(()) => Ok(()),
Err(error) => {
log::warn!(
"failed to move authorized replacement target to Trash, attempting hard delete: {}",
error
);
let current_metadata = match std::fs::symlink_metadata(target) {
Ok(metadata) => metadata,
Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()),
Err(error) => {
return Err(format!(
"Could not inspect replacement target '{}': {error}",
target.display()
));
}
};
validate_unowned_replacement_target(
app_handle,
target,
expected_fingerprint,
&current_metadata,
)?;
match std::fs::remove_file(target) {
Ok(()) => Ok(()),
Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()),
Err(error) => Err(error.to_string()),
}
}
};
match remove_result {
Ok(()) => return Ok(()),
Err(error) if attempt == 5 => {
return Err(format!(
"Could not remove replacement target '{}' after retries: {error}",
target.display()
));
}
Err(_) => {}
}
}
tokio::time::sleep(std::time::Duration::from_millis(200)).await;
}
Ok(())
}
fn expected_unadmitted_download_assets_present<R: tauri::Runtime>( fn expected_unadmitted_download_assets_present<R: tauri::Runtime>(
app_handle: &tauri::AppHandle<R>, app_handle: &tauri::AppHandle<R>,
persisted: &crate::ipc::DownloadItem, persisted: &crate::ipc::DownloadItem,
@@ -13416,7 +13617,8 @@ mod tests {
Aria2DaemonGuard, stale_lifecycle_cleanup_is_noop, Aria2DaemonGuard, stale_lifecycle_cleanup_is_noop,
should_skip_unowned_asset_cleanup, expected_unadmitted_download_assets_present, should_skip_unowned_asset_cleanup, expected_unadmitted_download_assets_present,
download_asset_or_sidecar_exists, download_asset_or_sidecar_exists,
classify_download_target, download_target_lock, classify_download_target, download_target_lock, inspect_unowned_replacement_target,
remove_unowned_replacement_target,
remove_download_assets_permanently, remove_download_container_assets_permanently, remove_download_assets_permanently, remove_download_container_assets_permanently,
restore_download_replacement, target_fingerprint, DownloadReplacementReservation, restore_download_replacement, target_fingerprint, DownloadReplacementReservation,
}; };
@@ -13602,6 +13804,100 @@ mod tests {
.unwrap()); .unwrap());
} }
#[test]
fn unowned_replacement_authorization_requires_the_original_target() {
use tauri::Manager;
let app = tauri::test::mock_builder()
.build(tauri::test::mock_context(tauri::test::noop_assets()))
.expect("mock app");
let storage_root = tempfile::tempdir_in(std::env::current_dir().unwrap()).unwrap();
let storage_layout = crate::storage::StorageLayout::resolve(
app.handle(),
crate::storage::StorageMode::Portable {
root: storage_root.path().to_path_buf(),
},
)
.unwrap();
app.manage(crate::db::init(&storage_layout).unwrap());
let download_root =
configure_test_download_root(app.handle(), &storage_root.path().join("downloads"));
let target = download_root.join("replace.bin");
std::fs::write(&target, b"original").unwrap();
let fingerprint = target_fingerprint(&target, &std::fs::symlink_metadata(&target).unwrap());
let item: crate::ipc::DownloadItem = serde_json::from_value(json!({
"id": "staged-replacement",
"url": "https://example.com/file.bin",
"fileName": "replace.bin",
"status": "staged",
"category": "Other",
"dateAdded": "",
"destination": download_root.to_string_lossy(),
"hasBeenDispatched": false,
"replaceExistingFingerprint": fingerprint
}))
.unwrap();
let authorized = inspect_unowned_replacement_target(app.handle(), &item, false)
.unwrap()
.expect("matching pre-admission replacement should be authorized");
assert_eq!(authorized.target, target);
let mut ambiguous_item = item.clone();
ambiguous_item.has_been_dispatched = None;
assert!(
inspect_unowned_replacement_target(app.handle(), &ambiguous_item, false)
.unwrap()
.is_none()
);
crate::download_ownership::set_primary_path(app.handle(), "other-owner", &target).unwrap();
let error = inspect_unowned_replacement_target(app.handle(), &item, false)
.expect_err("a Firelink-owned target must not use unowned replacement cleanup");
assert!(error.contains("owned by Firelink download other-owner"));
crate::download_ownership::remove(app.handle(), "other-owner").unwrap();
std::fs::write(&target, b"changed target").unwrap();
let error = inspect_unowned_replacement_target(app.handle(), &item, false)
.expect_err("a changed target must not inherit replacement authorization");
assert!(error.contains("changed after duplicate resolution"));
}
#[tokio::test]
async fn unowned_replacement_cleanup_removes_only_a_matching_primary_file() {
use tauri::Manager;
let app = tauri::test::mock_builder()
.build(tauri::test::mock_context(tauri::test::noop_assets()))
.expect("mock app");
let storage_root = tempfile::tempdir_in(std::env::current_dir().unwrap()).unwrap();
let storage_layout = crate::storage::StorageLayout::resolve(
app.handle(),
crate::storage::StorageMode::Portable {
root: storage_root.path().to_path_buf(),
},
)
.unwrap();
app.manage(crate::db::init(&storage_layout).unwrap());
let download_root =
configure_test_download_root(app.handle(), &storage_root.path().join("downloads"));
let target = download_root.join("replace.bin");
std::fs::write(&target, b"original").unwrap();
let fingerprint = target_fingerprint(&target, &std::fs::symlink_metadata(&target).unwrap());
remove_unowned_replacement_target(&target, &fingerprint, true, app.handle())
.await
.expect("matching replacement target should be removable");
assert!(!target.exists());
std::fs::write(&target, b"new file").unwrap();
let error = remove_unowned_replacement_target(&target, &fingerprint, true, app.handle())
.await
.expect_err("a replacement target with a new fingerprint must be preserved");
assert!(error.contains("changed after duplicate resolution"));
assert_eq!(std::fs::read(&target).unwrap(), b"new file");
}
#[test] #[test]
fn terminal_aria2_status_preserves_exact_progress_snapshot() { fn terminal_aria2_status_preserves_exact_progress_snapshot() {
let snapshot = aria2_download_state_progress(Some(&json!({ let snapshot = aria2_download_state_progress(Some(&json!({
+31
View File
@@ -408,6 +408,37 @@ describe('useDownloadProgressStore', () => {
release(); release();
}); });
it('invalidates replacement authorization when a native state event changes identity', async () => {
const handlers: Record<string, (event: any) => void> = {};
vi.mocked(ipc.listenEvent).mockImplementation((event, handler) => {
handlers[event] = handler as (event: any) => void;
return Promise.resolve(vi.fn());
});
useDownloadStore.setState({
downloads: [{
id: 'native-identity-change',
url: 'https://example.com/file.bin',
fileName: 'old.bin',
status: 'staged',
category: 'Other',
dateAdded: '',
replaceExistingFingerprint: 'original-target-fingerprint',
}],
});
const release = await initDownloadListener();
handlers['download-state']({ payload: {
id: 'native-identity-change',
status: 'ready',
error: null,
fileName: 'new.bin',
} });
expect(useDownloadStore.getState().downloads[0].fileName).toBe('new.bin');
expect(useDownloadStore.getState().downloads[0].replaceExistingFingerprint).toBeUndefined();
release();
});
it('keeps Aria2 connection telemetry from the live progress event', async () => { it('keeps Aria2 connection telemetry from the live progress event', async () => {
const handlers: Record<string, (event: any) => void> = {}; const handlers: Record<string, (event: any) => void> = {};
vi.mocked(ipc.listenEvent).mockImplementation((event, handler) => { vi.mocked(ipc.listenEvent).mockImplementation((event, handler) => {
+2
View File
@@ -455,9 +455,11 @@ const startDownloadListeners = async () => {
current.isTorrent === true, current.isTorrent === true,
current.category current.category
); );
updates.replaceExistingFingerprint = undefined;
} }
if (payload.destination && payload.destination !== current.destination) { if (payload.destination && payload.destination !== current.destination) {
updates.destination = payload.destination; updates.destination = payload.destination;
updates.replaceExistingFingerprint = undefined;
} }
if (status !== 'downloading' && status !== 'verifying') { if (status !== 'downloading' && status !== 'verifying') {
updates.speed = '-'; updates.speed = '-';
+22
View File
@@ -143,6 +143,28 @@ describe('useDownloadStore', () => {
expect(fileName.endsWith('.mp4')).toBe(true); expect(fileName.endsWith('.mp4')).toBe(true);
}); });
it('invalidates staged replacement authorization when its output identity changes', async () => {
useDownloadStore.setState({
downloads: [{
id: 'staged-replacement',
url: 'https://example.com/file.bin',
fileName: 'old.bin',
destination: '/tmp/downloads',
status: 'staged',
category: 'Other',
dateAdded: '',
replaceExistingFingerprint: 'original-target-fingerprint',
}] as any[],
});
await useDownloadStore.getState().applyProperties('staged-replacement', {
fileName: 'new.bin',
destination: '/tmp/other-downloads',
});
expect(useDownloadStore.getState().downloads[0].replaceExistingFingerprint).toBeUndefined();
});
it('rejects queued identity edits before invalidating their dispatch', async () => { it('rejects queued identity edits before invalidating their dispatch', async () => {
useDownloadStore.setState({ useDownloadStore.setState({
downloads: [{ downloads: [{
+5
View File
@@ -1245,6 +1245,7 @@ export const useDownloadStore = create<DownloadState>((set, get) => {
: hasCredentialMaterial( : hasCredentialMaterial(
Object.prototype.hasOwnProperty.call(updates, field) ? updates[field] : item[field] Object.prototype.hasOwnProperty.call(updates, field) ? updates[field] : item[field]
)); ));
const identityUpdated = updates.fileName !== undefined || updates.destination !== undefined;
const normalizedUpdates = { const normalizedUpdates = {
...(updates.fileName === undefined ...(updates.fileName === undefined
? updates ? updates
@@ -1263,6 +1264,10 @@ export const useDownloadStore = create<DownloadState>((set, get) => {
credentialsRequired: undefined, credentialsRequired: undefined,
} }
: {}), : {}),
// A replacement fingerprint authorizes one exact destination identity.
// Editing either part of that identity invalidates the authorization so
// it cannot be replayed against a different path after persistence.
...(identityUpdated ? { replaceExistingFingerprint: undefined } : {}),
}; };
const disablingTorrentRemoval = item.isTorrent === true const disablingTorrentRemoval = item.isTorrent === true
&& normalizedUpdates.torrentRemoveUnselectedFile === false && normalizedUpdates.torrentRemoveUnselectedFile === false