fix: address post-audit regressions across queue, db, and ui

- Preserved extension-captured cookies through the Add modal, with a clean fallback when captured cookies break metadata fetching.
- Prevented batched extension captures from losing URLs or reusing stale cookie/header contexts.
- Fixed pause/resume and enqueue generation races, including cancellation during queue reservation and replay after task removal.
- Made startup database initialization safe under React StrictMode.
- Serialized keyring operations and corrected Linux legacy migration/deletion behavior.
- Restored `Downloading` state after yt-dlp retries.
- Replaced hardcoded media heights with dynamically detected formats, including nonstandard qualities such as 576p and 2880p.
This commit is contained in:
NimBold
2026-07-10 12:07:25 +03:30
parent 3fbd0742be
commit 4f4c655de6
11 changed files with 550 additions and 186 deletions
+76 -6
View File
@@ -12,6 +12,7 @@ const CURRENT_SCHEMA_VERSION: i64 = 1;
const TOKEN_CHANGED_NOTICE: &str = "pairing-token-changed";
pub const PAIRING_TOKEN_KEYCHAIN_ID: &str = "extension-pairing-token";
const KEYCHAIN_SERVICE: &str = "com.firelink.app";
static KEYRING_OPERATION_LOCK: Mutex<()> = Mutex::new(());
pub struct DbState {
conn: Mutex<Connection>,
@@ -888,16 +889,16 @@ fn ensure_keyring_store() -> Result<(), String> {
#[cfg(target_os = "windows")]
{
let store = windows_native_keyring_store::Store::new()
.map_err(|error| error.to_string())?;
let store =
windows_native_keyring_store::Store::new().map_err(|error| error.to_string())?;
keyring_core::set_default_store(store);
Ok(())
}
#[cfg(target_os = "linux")]
{
let store = zbus_secret_service_keyring_store::Store::new()
.map_err(|error| error.to_string())?;
let store =
zbus_secret_service_keyring_store::Store::new().map_err(|error| error.to_string())?;
keyring_core::set_default_store(store);
Ok(())
}
@@ -934,29 +935,98 @@ fn keychain_entry(id: &str) -> Result<keyring_core::Entry, String> {
keychain_entry_with_target(id, None)
}
fn lock_keyring_operations() -> Result<std::sync::MutexGuard<'static, ()>, String> {
KEYRING_OPERATION_LOCK
.lock()
.map_err(|_| "keyring operation lock is unavailable".to_string())
}
#[cfg(target_os = "linux")]
fn legacy_linux_keychain_entries(id: &str) -> Result<Vec<keyring_core::Entry>, String> {
ensure_keyring_store()?;
let entries = keyring_core::Entry::search(&std::collections::HashMap::from([
("service", KEYCHAIN_SERVICE),
("username", id),
]))
.map_err(|error| error.to_string())?;
let mut legacy = Vec::new();
for entry in entries {
let attributes = entry.get_attributes().map_err(|error| error.to_string())?;
if !attributes.contains_key("target") {
legacy.push(entry);
}
}
Ok(legacy)
}
#[cfg(target_os = "linux")]
fn unique_legacy_linux_keychain_entry(id: &str) -> Result<Option<keyring_core::Entry>, String> {
let mut entries = legacy_linux_keychain_entries(id)?;
match entries.len() {
0 => Ok(None),
1 => Ok(entries.pop()),
count => Err(format!(
"Entry is matched by {count} legacy Linux credentials"
)),
}
}
pub fn set_keychain_password(id: &str, password: &str) -> Result<(), String> {
let _guard = lock_keyring_operations()?;
let entry = keychain_entry(id)?;
#[cfg(target_os = "linux")]
if let Err(error) = entry.get_credential() {
match error {
keyring_core::Error::NoEntry => {
if let Some(legacy) = unique_legacy_linux_keychain_entry(id)? {
return legacy
.set_password(password)
.map_err(|error| error.to_string());
}
}
error => return Err(error.to_string()),
}
}
entry
.set_password(password)
.map_err(|error| error.to_string())
}
pub fn get_keychain_password(id: &str) -> Result<String, String> {
let _guard = lock_keyring_operations()?;
let entry = keychain_entry(id)?;
match entry.get_password() {
Ok(password) => Ok(password),
#[cfg(target_os = "linux")]
Err(_) => keychain_entry_with_target(id, None)?
Err(keyring_core::Error::NoEntry) => unique_legacy_linux_keychain_entry(id)?
.ok_or_else(|| keyring_core::Error::NoEntry.to_string())?
.get_password()
.map_err(|error| error.to_string()),
#[cfg(target_os = "linux")]
Err(error) => Err(error.to_string()),
#[cfg(not(target_os = "linux"))]
Err(error) => Err(error.to_string()),
}
}
pub fn delete_keychain_password(id: &str) -> Result<(), String> {
let _guard = lock_keyring_operations()?;
let entry = keychain_entry(id)?;
let _ = entry.delete_credential();
match entry.delete_credential() {
Ok(()) | Err(keyring_core::Error::NoEntry) => {}
Err(error) => return Err(error.to_string()),
}
#[cfg(target_os = "linux")]
for legacy in legacy_linux_keychain_entries(id)? {
match legacy.delete_credential() {
Ok(()) | Err(keyring_core::Error::NoEntry) => {}
Err(error) => return Err(error.to_string()),
}
}
Ok(())
}
+9 -9
View File
@@ -315,13 +315,10 @@ fn normalize_download(payload: ExtensionRequest) -> Option<ExtensionDownload> {
silent: payload.silent,
filename,
headers: payload.headers.filter(|value| !value.trim().is_empty()),
// A full browser Cookie header can exceed upstream request-header
// limits. Media uses yt-dlp's configured browser-cookie source
// instead; regular captured downloads retain their exact cookies.
cookies: (!payload.media)
.then_some(payload.cookies)
.flatten()
.filter(|value| !value.trim().is_empty()),
// Keep the exact tab/container cookie context. The Add modal may retry
// public media without this header when an upstream rejects its size,
// but authenticated and container-scoped media must not lose it here.
cookies: payload.cookies.filter(|value| !value.trim().is_empty()),
media: payload.media,
})
}
@@ -499,7 +496,7 @@ mod tests {
}
#[test]
fn explicit_media_drops_the_extension_cookie_header() {
fn explicit_media_preserves_the_extension_cookie_header() {
let download = normalize_download(ExtensionRequest {
urls: vec!["https://www.youtube.com/watch?v=example".to_string()],
referer: None,
@@ -512,7 +509,10 @@ mod tests {
.expect("valid media handoff");
assert!(download.media);
assert!(download.cookies.is_none());
assert_eq!(
download.cookies.as_deref(),
Some("large=browser-cookie-header")
);
assert_eq!(download.headers.as_deref(), Some("User-Agent: Firefox"));
}
+165 -58
View File
@@ -3,7 +3,7 @@
// Learn more about Tauri commands at https://tauri.app/develop/calling-rust/
use regex::Regex;
use serde::Serialize;
use std::collections::{HashMap, VecDeque};
use std::collections::{BTreeSet, HashMap, VecDeque};
use std::hash::{Hash, Hasher};
use std::path::PathBuf;
use std::sync::OnceLock;
@@ -312,26 +312,43 @@ fn is_excluded_yt_dlp_format(value: &serde_json::Value) -> bool {
}
fn format_height(value: &serde_json::Value) -> Option<u64> {
if let Some(height) = json_u64(value, "height") {
if let Some(height) = json_u64(value, "height").filter(|height| *height > 0) {
return Some(height);
}
if let Some(resolution) = json_str(value, "resolution") {
if let Some((_, height)) = resolution.split_once('x') {
if let Ok(parsed) = height.parse::<u64>() {
return Some(parsed);
if let Some((_, height)) = resolution
.split_once('x')
.or_else(|| resolution.split_once('X'))
{
if let Ok(parsed) = height.trim().parse::<u64>() {
if parsed > 0 {
return Some(parsed);
}
}
}
}
let note = json_lower(value, "format_note");
for height in [4320_u64, 2160, 1440, 1080, 720, 480, 360, 240, 144] {
if note.contains(&format!("{height}p")) {
return Some(height);
let bytes = note.as_bytes();
let mut heights = Vec::new();
for (index, byte) in bytes.iter().enumerate() {
if *byte != b'p' || index == 0 {
continue;
}
let mut start = index;
while start > 0 && bytes[start - 1].is_ascii_digit() {
start -= 1;
}
if start < index {
if let Ok(height) = note[start..index].parse::<u64>() {
if height > 0 {
heights.push(height);
}
}
}
}
None
heights.into_iter().max()
}
fn matches_media_height(value: &serde_json::Value, target: u64) -> bool {
@@ -339,11 +356,6 @@ fn matches_media_height(value: &serde_json::Value, target: u64) -> bool {
return false;
}
let note = json_lower(value, "format_note");
if note.contains(&format!("{target}p")) {
return true;
}
format_height(value) == Some(target)
}
@@ -536,13 +548,13 @@ fn build_media_format_options(
let mut options = Vec::new();
if has_video {
let available_heights: Vec<u64> = [4320_u64, 2160, 1440, 1080, 720, 480, 360, 240, 144]
let available_heights: Vec<u64> = clean_formats
.iter()
.filter(|format| has_video_stream(format))
.filter_map(|format| format_height(format))
.collect::<BTreeSet<_>>()
.into_iter()
.filter(|height| {
clean_formats
.iter()
.any(|format| matches_media_height(format, *height))
})
.rev()
.collect();
for height in available_heights {
@@ -2882,11 +2894,11 @@ pub(crate) async fn start_media_download_internal(
let max_retries = max_tries.unwrap_or(0).max(0) as usize;
let mut strike = 0_usize;
let mut processing_started = false;
let mut effective_cookie_source = cookie_source.clone();
let mut browser_cookie_fallback_used = false;
while strike <= max_retries {
let mut processing_started = false;
let ytdlp_path = resolve_bundled_binary_path(&app_handle, "yt-dlp")?;
let mut cmd = app_handle.shell().command(&ytdlp_path);
for arg in media_progress_args() {
@@ -2986,6 +2998,16 @@ pub(crate) async fn start_media_download_internal(
let (mut rx, child) = cmd
.spawn()
.map_err(|e| format!("Failed to spawn yt-dlp: {}", e))?;
if strike > 0 {
// The backoff path emits `Retrying`. Restore the live transfer
// state when the replacement process actually starts so React
// accepts progress from this attempt instead of staying stuck.
progress_state.speed_sampler.reset();
let _ = app_handle.emit(
"download-state",
DownloadStateEvent::new(id, crate::ipc::DownloadStatus::Downloading),
);
}
log::info!("yt-dlp spawned for id: {} (strike {})", id, strike);
let mut stderr_tail = String::new();
@@ -3101,7 +3123,6 @@ pub(crate) async fn start_media_download_internal(
);
}
if !processing_started && is_media_processing_line(&line) {
processing_started = true;
let _ = app_handle.emit(
"download-state",
DownloadStateEvent::new(
@@ -3946,6 +3967,18 @@ async fn get_pending_order(
Ok(state.queue_manager.pending_order(queue_id.as_deref()).await)
}
fn enqueue_lifecycle_generation(item: &queue::EnqueueItem) -> Result<u64, String> {
item.lifecycle_generation
.as_deref()
.map(|generation| {
generation
.parse::<u64>()
.map_err(|_| "Invalid enqueue lifecycle generation".to_string())
})
.transpose()
.map(|generation| generation.unwrap_or_default())
}
#[tauri::command]
async fn enqueue_download(
app_handle: tauri::AppHandle,
@@ -3955,30 +3988,35 @@ async fn enqueue_download(
let id = item.id.clone();
item.filename = crate::download_ownership::canonical_download_filename(&item.filename);
let accepted_filename = item.filename.clone();
crate::download_ownership::register_expected(
let lifecycle_generation = enqueue_lifecycle_generation(&item).map_err(AppError::Internal)?;
let previous_generation = state
.queue_manager
.reserve_enqueue_generation(&id, lifecycle_generation)
.await
.map_err(AppError::Internal)?;
if let Err(error) = crate::download_ownership::register_expected(
&app_handle,
&item.id,
&item.destination,
&item.filename,
)?;
let lifecycle_generation = item
.lifecycle_generation
.as_deref()
.map(|generation| {
generation
.parse::<u64>()
.map_err(|_| AppError::Internal("Invalid enqueue lifecycle generation".to_string()))
})
.transpose()?
.unwrap_or_default();
if let Err(e) = state
) {
state
.queue_manager
.rollback_enqueue_reservation(&id, lifecycle_generation, previous_generation)
.await;
return Err(AppError::Internal(error));
}
if let Err(error) = state
.queue_manager
.push_with_generation(item.into_task(), lifecycle_generation)
.commit_reserved_enqueue(item.into_task(), lifecycle_generation)
.await
{
let _ = crate::download_ownership::remove(&app_handle, &id);
state.queue_manager.release_registered_id(&id).await;
return Err(AppError::Internal(e));
state
.queue_manager
.rollback_enqueue_reservation(&id, lifecycle_generation, previous_generation)
.await;
return Err(AppError::Internal(error));
}
Ok(crate::ipc::EnqueueAccepted {
id,
@@ -4006,30 +4044,83 @@ async fn cancel_enqueue_generation(
async fn enqueue_many(
app_handle: tauri::AppHandle,
state: tauri::State<'_, AppState>,
mut items: Vec<queue::EnqueueItem>,
items: Vec<queue::EnqueueItem>,
) -> Result<Vec<crate::ipc::EnqueueResult>, AppError> {
for item in &mut items {
let mut results = Vec::with_capacity(items.len());
for mut item in items {
item.filename = crate::download_ownership::canonical_download_filename(&item.filename);
}
for item in &items {
crate::download_ownership::register_expected(
let id = item.id.clone();
let filename = item.filename.clone();
let lifecycle_generation = match enqueue_lifecycle_generation(&item) {
Ok(generation) => generation,
Err(error) => {
results.push(crate::ipc::EnqueueResult {
id,
success: false,
filename: None,
error: Some(error),
});
continue;
}
};
let previous_generation = match state
.queue_manager
.reserve_enqueue_generation(&id, lifecycle_generation)
.await
{
Ok(previous) => previous,
Err(error) => {
results.push(crate::ipc::EnqueueResult {
id,
success: false,
filename: None,
error: Some(error),
});
continue;
}
};
if let Err(error) = crate::download_ownership::register_expected(
&app_handle,
&item.id,
&item.destination,
&item.filename,
)?;
}
let tasks = items
.into_iter()
.map(queue::EnqueueItem::into_task)
.collect();
let results = state.queue_manager.enqueue_many(tasks).await;
for result in &results {
if !result.success {
let _ = crate::download_ownership::remove(&app_handle, &result.id);
state.queue_manager.release_registered_id(&result.id).await;
) {
state
.queue_manager
.rollback_enqueue_reservation(&id, lifecycle_generation, previous_generation)
.await;
results.push(crate::ipc::EnqueueResult {
id,
success: false,
filename: None,
error: Some(error),
});
continue;
}
if let Err(error) = state
.queue_manager
.commit_reserved_enqueue(item.into_task(), lifecycle_generation)
.await
{
let _ = crate::download_ownership::remove(&app_handle, &id);
state
.queue_manager
.rollback_enqueue_reservation(&id, lifecycle_generation, previous_generation)
.await;
results.push(crate::ipc::EnqueueResult {
id,
success: false,
filename: None,
error: Some(error),
});
continue;
}
results.push(crate::ipc::EnqueueResult {
id,
success: true,
filename: Some(filename),
error: None,
});
}
Ok(results)
@@ -5048,7 +5139,7 @@ mod tests {
}
#[test]
fn keeps_low_and_ultra_high_available_video_qualities() {
fn keeps_every_available_video_height_including_nonstandard_qualities() {
let formats = vec![
json!({
"format_id": "401",
@@ -5063,12 +5154,28 @@ mod tests {
"height": 144,
"vcodec": "avc1.42E01E",
"acodec": "mp4a.40.2"
})
}),
json!({
"format_id": "five-k",
"ext": "webm",
"resolution": "5120X2880",
"vcodec": "av01.0.17M.08",
"acodec": "none"
}),
json!({
"format_id": "pal",
"ext": "mp4",
"format_note": "Premium 576p",
"vcodec": "avc1.4d401f",
"acodec": "none"
}),
];
let options = build_media_format_options(&formats, Some(60.0));
assert!(options.iter().any(|format| format.resolution == "4320p"));
assert!(options.iter().any(|format| format.resolution == "2880p"));
assert!(options.iter().any(|format| format.resolution == "576p"));
assert!(options.iter().any(|format| format.resolution == "144p"));
}
+81 -62
View File
@@ -85,6 +85,7 @@ pub trait SidecarSpawner: Send + Sync + 'static {
pub struct QueueManager<R: tauri::Runtime = tauri::Wry> {
registered_ids: Mutex<HashSet<String>>,
enqueue_cancellations: Mutex<HashMap<String, u64>>,
enqueue_generations: Mutex<HashMap<String, u64>>,
pending: Mutex<VecDeque<QueuedTask>>,
semaphore: Arc<Semaphore>,
active_permits: Mutex<HashMap<String, OwnedSemaphorePermit>>,
@@ -136,6 +137,7 @@ impl<R: tauri::Runtime> QueueManager<R> {
Self {
registered_ids: Mutex::new(HashSet::new()),
enqueue_cancellations: Mutex::new(HashMap::new()),
enqueue_generations: Mutex::new(HashMap::new()),
pending: Mutex::new(VecDeque::new()),
semaphore: Arc::new(Semaphore::new(capacity)),
active_permits: Mutex::new(HashMap::new()),
@@ -184,32 +186,99 @@ impl<R: tauri::Runtime> QueueManager<R> {
.or_insert(generation);
}
/// Atomically checks the cancellation watermark before registering a task.
pub async fn push_with_generation(
/// Atomically reserve an ID after rejecting cancelled or replayed generations.
/// The returned watermark must be passed to `rollback_enqueue_reservation`
/// if ownership registration fails before the task is committed.
pub async fn reserve_enqueue_generation(
&self,
id: &str,
generation: u64,
) -> Result<Option<u64>, String> {
let cancellations = self.enqueue_cancellations.lock().await;
if cancellations
.get(id)
.is_some_and(|cancelled| *cancelled >= generation)
{
return Err("Download enqueue was superseded by a newer user action".to_string());
}
let mut generations = self.enqueue_generations.lock().await;
let previous_generation = generations.get(id).copied();
if previous_generation.is_some_and(|seen| seen >= generation) {
return Err("Download enqueue was superseded by a newer user action".to_string());
}
let mut registered = self.registered_ids.lock().await;
if registered.contains(id) {
return Err("Duplicate task".to_string());
}
registered.insert(id.to_string());
generations.insert(id.to_string(), generation);
Ok(previous_generation)
}
pub async fn rollback_enqueue_reservation(
&self,
id: &str,
generation: u64,
previous_generation: Option<u64>,
) {
let mut generations = self.enqueue_generations.lock().await;
let mut registered = self.registered_ids.lock().await;
if generations.get(id).copied() != Some(generation) {
return;
}
registered.remove(id);
match previous_generation {
Some(previous) => {
generations.insert(id.to_string(), previous);
}
None => {
generations.remove(id);
}
}
}
pub async fn commit_reserved_enqueue(
&self,
task: QueuedTask,
generation: u64,
) -> Result<(), String> {
let id = task.id.clone();
let cancellations = self.enqueue_cancellations.lock().await;
if cancellations.get(&id).is_some_and(|cancelled| *cancelled >= generation) {
if cancellations
.get(&id)
.is_some_and(|cancelled| *cancelled >= generation)
{
return Err("Download enqueue was superseded by a newer user action".to_string());
}
let mut registered = self.registered_ids.lock().await;
if registered.contains(&id) {
return Err("Duplicate task".to_string());
}
registered.insert(id.clone());
drop(registered);
drop(cancellations);
self.pending.lock().await.push_back(task);
self.emit_state(id, DownloadStatus::Queued);
self.notify.notify_one();
Ok(())
}
/// Atomically checks the generation watermark before registering a task.
pub async fn push_with_generation(
&self,
task: QueuedTask,
generation: u64,
) -> Result<(), String> {
let id = task.id.clone();
let previous_generation = self.reserve_enqueue_generation(&id, generation).await?;
if let Err(error) = self.commit_reserved_enqueue(task, generation).await {
self.rollback_enqueue_reservation(&id, generation, previous_generation)
.await;
return Err(error);
}
Ok(())
}
/// Enqueue a task without a frontend lifecycle token. This is retained for
/// internal/test callers and still gets replay protection at generation 0.
pub async fn push(&self, task: QueuedTask) -> Result<(), String> {
self.push_with_generation(task, 0).await
}
pub async fn next_aria2_control_epoch(&self, id: &str) -> u64 {
let mut epochs = self.aria2_control_epochs.lock().await;
let epoch = epochs.get(id).copied().unwrap_or_default().wrapping_add(1);
@@ -235,22 +304,6 @@ impl<R: tauri::Runtime> QueueManager<R> {
self.aria2_retry_strikes.lock().await.contains_key(id)
}
/// Enqueue a task. Checks the centralized `registered_ids` for deduplication.
pub async fn push(&self, task: QueuedTask) -> Result<(), String> {
let id = task.id.clone();
let mut registered = self.registered_ids.lock().await;
if registered.contains(&id) {
return Err("Duplicate task".to_string());
}
registered.insert(id.clone());
drop(registered);
self.pending.lock().await.push_back(task);
self.emit_state(id, DownloadStatus::Queued);
self.notify.notify_one();
Ok(())
}
/// Pop the next task, or None if empty.
pub async fn pop_front(&self) -> Option<QueuedTask> {
self.pending.lock().await.pop_front()
@@ -878,40 +931,6 @@ impl<R: tauri::Runtime> QueueManager<R> {
}
removed
}
/// Bulk enqueue by appending tasks. Used by startup and start-all.
pub async fn enqueue_many(&self, tasks: Vec<QueuedTask>) -> Vec<crate::ipc::EnqueueResult> {
let mut results = Vec::new();
let mut registered = self.registered_ids.lock().await;
let mut pending = self.pending.lock().await;
for task in tasks {
let id = task.id.clone();
let filename = task.payload.filename.clone();
if registered.contains(&id) {
results.push(crate::ipc::EnqueueResult {
id: id.clone(),
success: false,
filename: None,
error: Some("Duplicate task".to_string()),
});
continue;
}
registered.insert(id.clone());
pending.push_back(task);
self.emit_state(id.clone(), DownloadStatus::Queued);
results.push(crate::ipc::EnqueueResult {
id,
success: true,
filename: Some(filename),
error: None,
});
}
drop(pending);
drop(registered);
self.notify.notify_one();
results
}
}
fn automatic_retry_limit(max_tries: Option<i32>) -> usize {