[Update] Make the update download atomic and its temp path unpredictable (#7627)

The downloader wrote to a predictable /tmp/<url-basename> path, ignored the
result of write(), and leaked the previous reply when a check was retried. It
also connected no error handler to the reply and had no transfer timeout.

Create the file inside a private QTemporaryDir, write it with QSaveFile and
commit it only after every byte was accepted, report reply errors, give the
request a 30 s transfer timeout, and delete the superseded reply. Ignore a
queued finished()/errorOccurred() from a reply that has already been replaced,
and drop the manual redirect branch because Qt follows redirects by default.

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

cockatrice/src/client/network/update/client/update_downloader.cpp

@@ -1,15 +1,16 @@
 #include "update_downloader.h"
 
-#include <QByteArray>
-#include <QChar>
 #include <QDir>
-#include <QFile>
+#include <QFileInfo>
 #include <QIODevice>
 #include <QNetworkAccessManager>
 #include <QNetworkRequest>
+#include <QSaveFile>
+#include <QTemporaryDir>
 #include <QUrl>
 #include <QVariant>
 #include <libcockatrice/network/network_manager.h>
+#include <utility>
 
 UpdateDownloader::UpdateDownloader(QObject *parent) : QObject(parent), response(nullptr)
 {
@@ -23,48 +24,116 @@ void UpdateDownloader::beginDownload(QUrl downloadUrl)
         originalUrl = downloadUrl;
     }
 
+    if (auto *old = std::exchange(response, nullptr)) {
+        old->abort();
+        old->deleteLater();
+    }
+
+    errorReported = false;
+    cancelRequested = false;
+
+    // The transfer timeout comes from the manager (NetworkManagerFactory).
     response = netMan->get(QNetworkRequest(downloadUrl));
     connect(response, &QNetworkReply::finished, this, &UpdateDownloader::fileFinished);
+    connect(response, &QNetworkReply::errorOccurred, this, &UpdateDownloader::downloadError);
     connect(response, &QNetworkReply::downloadProgress, this, &UpdateDownloader::downloadProgress);
-    connect(this, &UpdateDownloader::stopDownload, response, &QNetworkReply::abort);
+    connect(this, &UpdateDownloader::stopDownload, this, &UpdateDownloader::cancelDownload);
 }
 
-void UpdateDownloader::downloadError(QNetworkReply::NetworkError)
+void UpdateDownloader::downloadError(QNetworkReply::NetworkError networkError)
 {
-    if (response == nullptr) {
+    // A reply that was superseded by beginDownload() can still deliver a
+    // queued error; it must not be reported against the current reply.
+    if (response == nullptr || sender() != response || errorReported) {
         return;
     }
 
-    emit error(response->errorString().toUtf8());
+    // A transfer timeout aborts the reply, so it arrives as OperationCanceledError.
+    // Only an abort the user asked for is suppressed; every other cancel is a
+    // stalled transfer and has to be reported.
+    if (networkError == QNetworkReply::OperationCanceledError) {
+        if (cancelRequested) {
+            return;
+        }
+
+        errorReported = true;
+        emit error(tr("The download timed out. Check your connection and try again."));
+        return;
+    }
+
+    errorReported = true;
+    emit error(response->errorString());
+}
+
+void UpdateDownloader::cancelDownload()
+{
+    cancelRequested = true;
+    if (response != nullptr) {
+        response->abort();
+    }
 }
 
 void UpdateDownloader::fileFinished()
 {
-    // If we finished but there's a redirect, follow it
-    QVariant redirect = response->attribute(QNetworkRequest::RedirectionTargetAttribute);
-    if (!redirect.isNull()) {
-        beginDownload(redirect.toUrl());
+    // An aborted, superseded reply also emits finished(). Only the reply
+    // currently stored in response may be consumed.
+    if (response == nullptr || sender() != response) {
         return;
     }
 
-    // Handle any errors we had
-    if (response->error()) {
-        emit error(response->errorString());
+    // errorOccurred normally reports this first; keep the check for errors that
+    // only surface when the reply finishes.
+    if (response->error() != QNetworkReply::NoError) {
+        downloadError(response->error());
+        response->deleteLater();
+        response = nullptr;
         return;
     }
 
-    // Work out the file name of the download
-    QString fileName = QDir::temp().path() + QDir::separator() + originalUrl.toString().section('/', -1);
+    // Give the update its own private directory. The name is unpredictable, so a
+    // local user cannot pre-create or symlink the path, and the original file
+    // name (including the installer suffix) is preserved inside it.
+    QTemporaryDir downloadDir(QDir::temp().filePath(QStringLiteral("cockatrice-update-XXXXXX")));
+    if (!downloadDir.isValid()) {
+        emit error(tr("Could not create a temporary directory for the download."));
+        response->deleteLater();
+        response = nullptr;
+        return;
+    }
+    downloadDir.setAutoRemove(false);
 
-    // Save the build in a temporary directory
-    QFile file(fileName);
+    const QString remoteName = QFileInfo(originalUrl.path()).fileName();
+    const QString fileName = downloadDir.filePath(remoteName.isEmpty() ? QStringLiteral("update") : remoteName);
+
+    QSaveFile file(fileName);
     if (!file.open(QIODevice::WriteOnly)) {
-        emit error(tr("Could not open the file for reading."));
+        downloadDir.remove();
+        emit error(tr("Could not open the file for writing: %1").arg(file.errorString()));
+        response->deleteLater();
+        response = nullptr;
         return;
     }
 
-    file.write(response->readAll());
-    file.close();
+    const QByteArray data = response->readAll();
+    if (file.write(data) != data.size()) {
+        file.cancelWriting();
+        downloadDir.remove();
+        emit error(tr("Could not write the downloaded file to disk."));
+        response->deleteLater();
+        response = nullptr;
+        return;
+    }
+
+    if (!file.commit()) {
+        downloadDir.remove();
+        emit error(tr("Could not save the downloaded file: %1").arg(file.errorString()));
+        response->deleteLater();
+        response = nullptr;
+        return;
+    }
+
+    response->deleteLater();
+    response = nullptr;
 
     // Emit the success signal with a URL to the download file
     emit downloadSuccessful(QUrl::fromLocalFile(fileName));

cockatrice/src/client/network/update/client/update_downloader.h

@@ -32,10 +32,13 @@ private:
     QUrl originalUrl;
     QNetworkAccessManager *netMan;
     QNetworkReply *response;
+    bool errorReported = false;
+    bool cancelRequested = false;
 private slots:
     void fileFinished();
     void downloadProgress(qint64 bytesRead, qint64 totalBytes);
     void downloadError(QNetworkReply::NetworkError);
+    void cancelDownload();
 };
 
 #endif // COCKATRICE_UPDATEDOWNLOADER_H