fix: resolve multiple critical bugs identified in code review

- fix(engine): resolve aria2 port TOCTOU race and secure conf file

- fix(server): eliminate main queue deadlock risk in local extension server

- fix(controller): ensure synchronous state save on app termination

- fix(ui): fix table skipping new items by diffing identity instead of count

- fix(scheduler): use 2-letter abbreviations to resolve day name collisions

- fix(settings): purge keychain orphans when primer access is denied

- fix(metadata): prevent synchronous DNS resolution from blocking async pool

- refactor: clean up dead code and unused variables
This commit is contained in:
NimBold
2026-06-10 19:25:06 +03:30
parent fdbacb8a7f
commit 2a5452b7c6
8 changed files with 189 additions and 104 deletions
+1 -1
View File
@@ -322,7 +322,7 @@ struct AddDownloadsView: View {
ProgressView().controlSize(.small)
Text("Fetching media options...")
.foregroundStyle(.secondary)
} else if case .failed(let error) = firstMedia.state {
} else if case .failed(_) = firstMedia.state {
Image(systemName: "exclamationmark.triangle.fill").foregroundStyle(.red)
Text("Failed to load options.")
.foregroundStyle(.secondary)
+1 -1
View File
@@ -445,7 +445,7 @@ final class AppSettings: ObservableObject {
extensionPairingToken = Self.generateSecureToken()
}
} else {
isKeychainAccessGranted = false
revokeKeychainAccess()
}
}
+85 -46
View File
@@ -2,7 +2,7 @@ import Foundation
import CFNetwork
import Network
final class Aria2DownloadEngine {
final class Aria2DownloadEngine: Sendable {
struct Handle {
let processIdentifier: Int32
let rpcPort: Int
@@ -107,7 +107,7 @@ final class Aria2DownloadEngine {
speedLimitKiBPerSecond: Int?,
progress: @escaping @Sendable (DownloadProgress) -> Void,
completion: @escaping @Sendable (Result<Void, Error>) -> Void
) throws -> Handle {
) async throws -> Handle {
guard let executableURL else {
throw EngineError.executableNotFound
}
@@ -117,30 +117,48 @@ final class Aria2DownloadEngine {
withIntermediateDirectories: true
)
let rpcPort = Self.findFreePort()
let rpcSecret = UUID().uuidString
let confURL = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent("firelink-aria2-\(UUID().uuidString).conf")
do {
let confContent = "rpc-secret=\(rpcSecret)\n"
try confContent.write(to: confURL, atomically: true, encoding: .utf8)
try FileManager.default.setAttributes([.posixPermissions: 0o600], ofItemAtPath: confURL.path)
} catch {
throw EngineError.launchFailed("Could not write secure configuration file: \(error.localizedDescription)")
}
var lastError: Error?
let process = Process()
process.executableURL = executableURL
process.arguments = try arguments(
for: item,
proxyConfiguration: proxyConfiguration,
speedLimitKiBPerSecond: speedLimitKiBPerSecond,
rpcPort: rpcPort,
confURL: confURL
)
for _ in 1...5 {
let rpcPort = Self.findFreePort()
let rpcSecret = UUID().uuidString
let tempDir = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent("firelink-aria2-\(UUID().uuidString)")
do {
try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true, attributes: [.posixPermissions: 0o700])
} catch {
lastError = EngineError.launchFailed("Could not create secure temporary directory: \(error.localizedDescription)")
continue
}
let confURL = tempDir.appendingPathComponent("aria2.conf")
do {
let confContent = "rpc-secret=\(rpcSecret)\n"
try confContent.write(to: confURL, atomically: true, encoding: .utf8)
} catch {
lastError = EngineError.launchFailed("Could not write secure configuration file: \(error.localizedDescription)")
continue
}
let inputPipe = Pipe()
let outputPipe = Pipe()
let errorPipe = Pipe()
let process = Process()
process.executableURL = executableURL
do {
process.arguments = try arguments(
for: item,
proxyConfiguration: proxyConfiguration,
speedLimitKiBPerSecond: speedLimitKiBPerSecond,
rpcPort: rpcPort,
confURL: confURL
)
} catch {
lastError = error
break
}
let inputPipe = Pipe()
let outputPipe = Pipe()
let errorPipe = Pipe()
process.standardInput = inputPipe
process.standardOutput = outputPipe
process.standardError = errorPipe
@@ -169,7 +187,7 @@ final class Aria2DownloadEngine {
}
process.terminationHandler = { finishedProcess in
try? FileManager.default.removeItem(at: confURL)
try? FileManager.default.removeItem(at: tempDir)
completionMonitor.cancel()
outputPipe.fileHandleForReading.readabilityHandler = nil
errorPipe.fileHandleForReading.readabilityHandler = nil
@@ -190,33 +208,54 @@ final class Aria2DownloadEngine {
completionGate.complete(.failure(EngineError.launchFailed(message.isEmpty ? "exit code \(finishedProcess.terminationStatus)" : message)))
}
do {
try process.run()
if let input = inputFileContent(for: item).data(using: .utf8) {
inputPipe.fileHandleForWriting.write(input)
var didThrow = false
do {
try process.run()
if let input = inputFileContent(for: item).data(using: .utf8) {
inputPipe.fileHandleForWriting.write(input)
}
inputPipe.fileHandleForWriting.closeFile()
} catch {
didThrow = true
lastError = EngineError.launchFailed(error.localizedDescription)
}
if didThrow {
try? FileManager.default.removeItem(at: tempDir)
continue
}
try? await Task.sleep(nanoseconds: 100_000_000)
if !process.isRunning {
let stderr = String(data: errorBuffer.data, encoding: .utf8) ?? ""
if stderr.contains("Address already in use") || stderr.contains("Failed to bind") || stderr.contains("bind: Address") {
try? FileManager.default.removeItem(at: tempDir)
continue
}
// If it exited for another reason, we might still want to fail or let the terminationHandler process it.
// But the terminationHandler will hit completionGate, so we just return the handle.
}
inputPipe.fileHandleForWriting.closeFile()
} catch {
try? FileManager.default.removeItem(at: confURL)
throw EngineError.launchFailed(error.localizedDescription)
}
completionMonitor.set(
Self.monitorCompletion(
rpcPort: rpcPort,
rpcSecret: rpcSecret,
process: process,
completionGate: completionGate
completionMonitor.set(
Self.monitorCompletion(
rpcPort: rpcPort,
rpcSecret: rpcSecret,
process: process,
completionGate: completionGate
)
)
)
return Handle(processIdentifier: process.processIdentifier, rpcPort: rpcPort, rpcSecret: rpcSecret) {
completionMonitor.cancel()
if process.isRunning {
process.terminate()
return Handle(processIdentifier: process.processIdentifier, rpcPort: rpcPort, rpcSecret: rpcSecret) {
completionMonitor.cancel()
if process.isRunning {
process.terminate()
}
try? FileManager.default.removeItem(at: tempDir)
}
try? FileManager.default.removeItem(at: confURL)
}
throw lastError ?? EngineError.launchFailed("Failed to start aria2c after 5 attempts.")
}
private static func monitorCompletion(
+67 -44
View File
@@ -74,7 +74,7 @@ final class DownloadController: ObservableObject {
NotificationCenter.default.publisher(for: NSApplication.willTerminateNotification)
.sink { [weak self] _ in
self?.saveDownloads()
self?.saveDownloadsSync()
}
.store(in: &cancellables)
@@ -371,12 +371,7 @@ final class DownloadController: ObservableObject {
pumpQueue()
}
func remove(at offsets: IndexSet, deleteFiles: Bool = false) {
for index in offsets {
let item = downloads[index]
delete(item, deleteFiles: deleteFiles)
}
}
func delete(_ item: DownloadItem, deleteFiles: Bool = false) {
activeHandles[item.id]?.cancel()
@@ -598,49 +593,61 @@ final class DownloadController: ObservableObject {
}
}
} else {
do {
let handle = try engine.start(
item: injectedEngineItem(from: item),
proxyConfiguration: settings.downloadProxyConfiguration,
speedLimitKiBPerSecond: effectiveSpeedLimitKiBPerSecond(for: item),
progress: { [weak self] progress in
Task { @MainActor in
let now = Date()
if let last = self?.lastProgressUpdateTimes[item.id], now.timeIntervalSince(last) < 0.25 {
return
Task {
do {
let liveItem = injectedEngineItem(from: item)
let handle = try await engine.start(
item: liveItem,
proxyConfiguration: settings.downloadProxyConfiguration,
speedLimitKiBPerSecond: effectiveSpeedLimitKiBPerSecond(for: liveItem),
progress: { [weak self] progress in
Task { @MainActor in
let now = Date()
if let last = self?.lastProgressUpdateTimes[item.id], now.timeIntervalSince(last) < 0.25 {
return
}
self?.lastProgressUpdateTimes[item.id] = now
self?.update(item.id) {
guard $0.status == .downloading else { return }
$0.progress = progress.fraction
$0.bytesText = progress.bytesText
$0.speedText = progress.speedText
$0.etaText = progress.etaText
$0.connectionCount = progress.connectionCount
$0.message = "Downloading"
}
}
self?.lastProgressUpdateTimes[item.id] = now
self?.update(item.id) {
guard $0.status == .downloading else { return }
$0.progress = progress.fraction
$0.bytesText = progress.bytesText
$0.speedText = progress.speedText
$0.etaText = progress.etaText
$0.connectionCount = progress.connectionCount
$0.message = "Downloading"
},
completion: { [weak self] result in
Task { @MainActor in
self?.handleCompletion(item: item, result: result)
}
}
},
completion: { [weak self] result in
Task { @MainActor in
self?.handleCompletion(item: item, result: result)
)
await MainActor.run {
guard activeDownloadItem(id: item.id) != nil else {
handle.cancel()
return
}
activeHandles[item.id] = handle
update(item.id) {
$0.rpcPort = handle.rpcPort
$0.rpcSecret = handle.rpcSecret
$0.message = "Starting..."
}
saveDownloads()
applySpeedLimitsToActiveDownloads()
updateSleepActivity()
}
} catch {
await MainActor.run {
handleDownloadFailure(itemID: item.id, error: error)
applySpeedLimitsToActiveDownloads()
updateSleepActivity()
pumpQueue()
}
)
activeHandles[item.id] = handle
update(item.id) {
$0.rpcPort = handle.rpcPort
$0.rpcSecret = handle.rpcSecret
$0.message = "Starting..."
}
saveDownloads()
applySpeedLimitsToActiveDownloads()
updateSleepActivity()
} catch {
handleDownloadFailure(itemID: item.id, error: error)
applySpeedLimitsToActiveDownloads()
updateSleepActivity()
pumpQueue()
}
}
}
@@ -996,6 +1003,22 @@ final class DownloadController: ObservableObject {
}
}
private func saveDownloadsSync() {
let queuesCopy = queues
let downloadsCopy = downloads.map(\.redactedForPersistence)
let storageURL = self.storageURL
do {
let directory = storageURL.deletingLastPathComponent()
try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true, attributes: nil)
let state = StoredDownloadState(queues: queuesCopy, downloads: downloadsCopy)
let data = try JSONEncoder().encode(state)
try data.write(to: storageURL, options: .atomic)
} catch {
print("Failed to synchronously save downloads: \(error)")
}
}
private func saveDownloads() {
let queuesCopy = queues
let downloadsCopy = downloads.map(\.redactedForPersistence)
@@ -54,9 +54,14 @@ enum DownloadMetadataFetcher {
return pending
}
if isAutoFetch, let host = url.host, isPrivateHost(host) {
pending.state = .loaded
return pending
if isAutoFetch, let host = url.host {
let isPrivate = await Task.detached {
isPrivateHost(host)
}.value
if isPrivate {
pending.state = .loaded
return pending
}
}
var request = URLRequest(url: url)
+3 -1
View File
@@ -126,7 +126,9 @@ struct DownloadTable: View {
}
.onAppear { sortedItems = items.sorted(using: sortOrder) }
.onChange(of: items) { _, newItems in
if newItems.count != sortedItems.count {
let existingIDs = Set(sortedItems.map(\.id))
let newIDs = Set(newItems.map(\.id))
if existingIDs != newIDs {
sortedItems = newItems.sorted(using: sortOrder)
} else {
let itemsDict = Dictionary(uniqueKeysWithValues: newItems.map { ($0.id, $0) })
+17 -1
View File
@@ -1,6 +1,7 @@
import Foundation
import Network
import AppKit
import Combine
final class LocalExtensionServer: @unchecked Sendable {
private enum Constants {
@@ -16,7 +17,12 @@ final class LocalExtensionServer: @unchecked Sendable {
private let settings: AppSettings
private let queue = DispatchQueue(label: "local.firelink.server")
let port: UInt16
private let tokenLock = NSLock()
private var _pairingToken: String = ""
private var cancellables = Set<AnyCancellable>()
@MainActor
init?(downloadController: DownloadController, settings: AppSettings) {
self.downloadController = downloadController
self.settings = settings
@@ -42,6 +48,16 @@ final class LocalExtensionServer: @unchecked Sendable {
self.listener = createdListener
self.port = selectedPort ?? 6412
settings.$extensionPairingToken
.sink { [weak self] token in
self?.tokenLock.withLock { self?._pairingToken = token }
}
.store(in: &cancellables)
}
private var currentPairingToken: String {
tokenLock.withLock { _pairingToken }
}
func start() {
@@ -139,7 +155,7 @@ final class LocalExtensionServer: @unchecked Sendable {
return isAllowedExtensionOrigin(request.header(named: "origin") ?? "") ? .noContent : .forbidden
}
let expectedToken = DispatchQueue.main.sync { settings.extensionPairingToken }
let expectedToken = currentPairingToken
guard let token = request.header(named: Constants.extensionRequestHeader),
token == expectedToken else {
return .forbidden
+7 -7
View File
@@ -18,13 +18,13 @@ enum SchedulerDay: Int, Codable, CaseIterable, Identifiable {
var shortName: String {
switch self {
case .sunday: "S"
case .monday: "M"
case .tuesday: "T"
case .wednesday: "W"
case .thursday: "T"
case .friday: "F"
case .saturday: "S"
case .sunday: "Su"
case .monday: "Mo"
case .tuesday: "Tu"
case .wednesday: "We"
case .thursday: "Th"
case .friday: "Fr"
case .saturday: "Sa"
}
}
}