Fix Deckstats and TappedOut request lifetime and result handling (#7705)

* Fix deck analysis request lifetime, result URLs and form encoding [skip ci]

* Preserve unknown cards in deck analysis exports

* Share deck analysis URL validation and drop the unused request latch

Both analysis interfaces carried a copy of normalizeAnalysisUrl; move it to one
header next to them. They are constructed per analysis and analyzeDeck is called
once, so the analysisStarted latch was unreachable; remove it from both.

---------

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

cockatrice/src/client/network/interfaces/deck_analysis_url.h

@@ -0,0 +1,42 @@
+/**
+ * @file deck_analysis_url.h
+ * @ingroup ApiInterfaces
+ */
+
+#ifndef DECK_ANALYSIS_URL_H
+#define DECK_ANALYSIS_URL_H
+
+#include <QString>
+#include <QUrl>
+
+/**
+ * Validates a deck address returned by a deck analysis service.
+ *
+ * Only the expected host (optionally "www."-prefixed), HTTPS, default ports and
+ * URLs without user information are accepted. Legacy HTTP addresses for the same
+ * service are upgraded to HTTPS in place instead of being rejected.
+ *
+ * Relative addresses must be resolved against the response URL by the caller.
+ *
+ * @return true when @p url names a deck on @p serviceHost; false otherwise.
+ */
+inline bool normalizeAnalysisUrl(QUrl &url, const QString &serviceHost)
+{
+    const QString host = url.host().toLower();
+    const bool legacyHttp = url.scheme() == QStringLiteral("http");
+    const int defaultPort = legacyHttp ? 80 : 443;
+    if (!url.isValid() || (!legacyHttp && url.scheme() != QStringLiteral("https")) || !url.userInfo().isEmpty() ||
+        !url.path().startsWith('/') || (url.port() != -1 && url.port() != defaultPort) ||
+        (host != serviceHost && host != QStringLiteral("www.") + serviceHost)) {
+        return false;
+    }
+    // Both services accept HTTPS. Older public metadata may still identify the
+    // same deck with HTTP; retain its host/path/query while using HTTPS to open it.
+    if (legacyHttp) {
+        url.setScheme(QStringLiteral("https"));
+        url.setPort(-1);
+    }
+    return true;
+}
+
+#endif

cockatrice/src/client/network/interfaces/deck_stats_interface.cpp

@@ -1,5 +1,6 @@
 #include "deck_stats_interface.h"
 
+#include "deck_analysis_url.h"
 #include "libcockatrice/card/card_info.h"
 #include "libcockatrice/card/database/card_database_querier.h"
 #include "libcockatrice/deck_list/tree/inner_deck_list_node.h"
@@ -10,12 +11,14 @@
 #include <QNetworkAccessManager>
 #include <QNetworkReply>
 #include <QNetworkRequest>
+#include <QPointer>
 #include <QRegularExpression>
 #include <QRegularExpressionMatch>
 #include <QSharedPointer>
 #include <QString>
+#include <QTextDocumentFragment>
 #include <QUrl>
-#include <QUrlQuery>
+#include <QWidget>
 #include <functional>
 #include <libcockatrice/card/database/card_database_manager.h>
 #include <libcockatrice/deck_list/deck_list.h>
@@ -29,30 +32,56 @@ DeckStatsInterface::DeckStatsInterface(QObject *parent) : QObject(parent)
     connect(manager, &QNetworkAccessManager::finished, this, &DeckStatsInterface::queryFinished);
 }
 
+void DeckStatsInterface::showError(const QString &message)
+{
+    // The notice belongs to the originating editor, not this one-shot request.
+    // Retire the request before opening UI: opening a window may reenter Qt.
+    QWidget *owner = qobject_cast<QWidget *>(parent());
+    auto *notice = new QMessageBox(QMessageBox::Critical, tr("Deck analysis"), message, QMessageBox::Ok, owner);
+    notice->setTextFormat(Qt::PlainText);
+    notice->setTextInteractionFlags(Qt::TextSelectableByMouse | Qt::TextSelectableByKeyboard);
+    notice->setAttribute(Qt::WA_DeleteOnClose);
+    deleteLater();
+    notice->open();
+}
+
 void DeckStatsInterface::queryFinished(QNetworkReply *reply)
 {
+    if (replyHandled) {
+        return;
+    }
+    replyHandled = true;
+    // All terminal paths release the reply before showing UI or invoking an
+    // external URL handler, either of which may destroy the originating editor.
+    reply->deleteLater();
     if (reply->error() != QNetworkReply::NoError) {
-        QMessageBox::critical(nullptr, tr("Error"), reply->errorString());
-        reply->deleteLater();
-        deleteLater();
+        showError(reply->errorString());
         return;
     }
 
-    QString data(reply->readAll());
-    reply->deleteLater();
-
-    static const QRegularExpression rx("<meta property=\"og:url\" content=\"([^\"]+)\"");
-    auto match = rx.match(data);
+    const QString data = QString::fromUtf8(reply->readAll());
+    static const QRegularExpression rx(
+        R"(<meta\b(?=[^>]*\bproperty\s*=\s*(['"])og:url\1)[^>]*\bcontent\s*=\s*(['"])(.*?)\2)",
+        QRegularExpression::CaseInsensitiveOption | QRegularExpression::DotMatchesEverythingOption);
+    const auto match = rx.match(data);
     if (!match.hasMatch()) {
-        QMessageBox::critical(nullptr, tr("Error"), tr("The reply from the server could not be parsed."));
-        deleteLater();
+        showError(tr("The reply from Deckstats did not contain a deck address."));
         return;
     }
 
-    QString deckUrl = match.captured(1);
-    QDesktopServices::openUrl(deckUrl);
+    const QString address = QTextDocumentFragment::fromHtml(match.captured(3)).toPlainText();
+    QUrl deckUrl = reply->url().resolved(QUrl(address, QUrl::StrictMode));
+    if (address.trimmed().isEmpty() || !normalizeAnalysisUrl(deckUrl, QStringLiteral("deckstats.net"))) {
+        showError(tr("The reply from Deckstats contained an invalid deck address."));
+        return;
+    }
 
+    QPointer<DeckStatsInterface> guard(this);
     deleteLater();
+    if (!QDesktopServices::openUrl(deckUrl) && guard) {
+        showError(tr("Could not open your browser. Open this address to see the analyzed deck:\n%1")
+                      .arg(deckUrl.toDisplayString()));
+    }
 }
 
 void DeckStatsInterface::getAnalyzeRequestData(const DeckList &deck, QByteArray &data)
@@ -60,12 +89,8 @@ void DeckStatsInterface::getAnalyzeRequestData(const DeckList &deck, QByteArray
     DeckList deckWithoutTokens;
     copyDeckWithoutTokens(deck, deckWithoutTokens);
 
-    QUrl params;
-    QUrlQuery urlQuery;
-    urlQuery.addQueryItem("deck", deckWithoutTokens.writeToString_Plain());
-    urlQuery.addQueryItem("decktitle", deck.getName());
-    params.setQuery(urlQuery);
-    data.append(params.query(QUrl::EncodeReserved).toUtf8());
+    data.append("deck=" + QUrl::toPercentEncoding(deckWithoutTokens.writeToString_Plain()));
+    data.append("&decktitle=" + QUrl::toPercentEncoding(deck.getName()));
 }
 
 void DeckStatsInterface::analyzeDeck(const DeckList &deck)
@@ -84,7 +109,9 @@ void DeckStatsInterface::copyDeckWithoutTokens(const DeckList &source, DeckList
 {
     auto copyIfNotAToken = [&destination](const auto node, const auto card) {
         CardInfoPtr dbCard = CardDatabaseManager::query()->getCardInfo(card->getName());
-        if (dbCard && !dbCard->getIsToken() && node->getName() != DECK_ZONE_MAYBEBOARD) {
+        // Missing local card data is not evidence that a card is a token. Keep
+        // its original name and quantity so the service analyzes the whole deck.
+        if ((!dbCard || !dbCard->getIsToken()) && node->getName() != DECK_ZONE_MAYBEBOARD) {
             DecklistCardNode *addedCard = destination.addCard(card->getName(), node->getName(), -1);
             addedCard->setNumber(card->getNumber());
         }

cockatrice/src/client/network/interfaces/deck_stats_interface.h

@@ -11,6 +11,7 @@
 #include <qtmetamacros.h>
 
 class QByteArray;
+class QString;
 class QNetworkAccessManager;
 class QNetworkReply;
 class DeckList;
@@ -20,6 +21,8 @@ class DeckStatsInterface : public QObject
     Q_OBJECT
 private:
     QNetworkAccessManager *manager;
+    bool replyHandled = false;
+    void showError(const QString &message);
 
     /**
      * Deckstats doesn't recognize token cards, and instead tries to find the

cockatrice/src/client/network/interfaces/tapped_out_interface.cpp

@@ -1,5 +1,6 @@
 #include "tapped_out_interface.h"
 
+#include "deck_analysis_url.h"
 #include "libcockatrice/card/card_info.h"
 #include "libcockatrice/card/database/card_database_querier.h"
 #include "libcockatrice/deck_list/tree/inner_deck_list_node.h"
@@ -13,15 +14,17 @@
 #include <QNetworkAccessManager>
 #include <QNetworkReply>
 #include <QNetworkRequest>
+#include <QPointer>
 #include <QRegularExpression>
 #include <QRegularExpressionMatch>
 #include <QRegularExpressionMatchIterator>
 #include <QSharedPointer>
 #include <QString>
 #include <QStringList>
+#include <QTextDocumentFragment>
 #include <QUrl>
-#include <QUrlQuery>
 #include <QVariant>
+#include <QWidget>
 #include <functional>
 #include <libcockatrice/card/database/card_database_manager.h>
 #include <libcockatrice/deck_list/deck_list.h>
@@ -35,54 +38,67 @@ TappedOutInterface::TappedOutInterface(QObject *parent) : QObject(parent)
     connect(manager, &QNetworkAccessManager::finished, this, &TappedOutInterface::queryFinished);
 }
 
+void TappedOutInterface::showError(const QString &message)
+{
+    // The notice belongs to the originating editor, not this one-shot request.
+    // Retire the request before opening UI: opening a window may reenter Qt.
+    QWidget *owner = qobject_cast<QWidget *>(parent());
+    auto *notice = new QMessageBox(QMessageBox::Critical, tr("Deck analysis"), message, QMessageBox::Ok, owner);
+    notice->setTextFormat(Qt::PlainText);
+    notice->setTextInteractionFlags(Qt::TextSelectableByMouse | Qt::TextSelectableByKeyboard);
+    notice->setAttribute(Qt::WA_DeleteOnClose);
+    deleteLater();
+    notice->open();
+}
+
 void TappedOutInterface::queryFinished(QNetworkReply *reply)
 {
-    if (reply->error() != QNetworkReply::NoError) {
-        QMessageBox::critical(nullptr, tr("Error"), reply->errorString());
+    if (replyHandled) {
+        return;
+    }
+    replyHandled = true;
+    // All terminal paths release the reply before showing UI or invoking an
+    // external URL handler, either of which may destroy the originating editor.
     reply->deleteLater();
-        deleteLater();
+    if (reply->error() != QNetworkReply::NoError) {
+        showError(reply->errorString());
         return;
     }
 
-    int httpStatus = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
-    if (reply->hasRawHeader("Location")) {
-        /*
-         * If the reply contains a "Location" header, a relative URL to the deck on TappedOut
-         * can be extracted from the header. The http status is a 302 "redirect".
-         */
-        QString deckUrl = reply->rawHeader("Location");
-        qCInfo(TappedOutInterfaceLog) << "Tappedout: good reply, http status" << httpStatus << "location" << deckUrl;
-        QDesktopServices::openUrl("https://tappedout.net" + deckUrl);
-    } else {
-        /*
-         * Otherwise, the deck has not been parsed correctly. Error messages can be extracted
-         * from the html. Css pseudo selector for errors: $("div.alert-danger > ul > li")
-         */
-        QString data(reply->readAll());
-        QStringList errorMessageList = {tr("Unable to analyze the deck.")};
-
-        static const QRegularExpression rx("<div class=\"alert alert-danger.*?<ul>(.*?)</ul>");
-        auto match = rx.match(data);
-        if (match.hasMatch()) {
-            QString errors = match.captured(1);
-            static const QRegularExpression rx2("<li>(.*?)</li>");
-            static const QRegularExpression rxremove("<[^>]*>");
-            auto matchIterator = rx2.globalMatch(errors);
-            while (matchIterator.hasNext()) {
-                auto match2 = matchIterator.next();
-                errorMessageList.append(match2.captured(1).remove(rxremove).simplified());
+    const int httpStatus = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt();
+    if (httpStatus >= 200 && httpStatus < 400 && reply->hasRawHeader("Location")) {
+        const QByteArray location = reply->rawHeader("Location");
+        QUrl deckUrl = reply->url().resolved(QUrl::fromEncoded(location, QUrl::StrictMode));
+        if (location.trimmed().isEmpty() || !normalizeAnalysisUrl(deckUrl, QStringLiteral("tappedout.net"))) {
+            showError(tr("The reply from TappedOut contained an invalid deck address."));
+            return;
         }
-        }
-
-        QString errorMessage = errorMessageList.join("\n");
-        qCWarning(TappedOutInterfaceLog) << "Tappedout: bad reply, http status" << httpStatus << "size" << data.size()
-                                         << "message" << errorMessage;
-
-        QMessageBox::critical(nullptr, tr("Error"), errorMessage);
-    }
-
-    reply->deleteLater();
+        QPointer<TappedOutInterface> guard(this);
         deleteLater();
+        if (!QDesktopServices::openUrl(deckUrl) && guard) {
+            showError(tr("Could not open your browser. Open this address to see the analyzed deck:\n%1")
+                          .arg(deckUrl.toDisplayString()));
+        }
+        return;
+    }
+
+    const QString data = QString::fromUtf8(reply->readAll());
+    QStringList errorMessageList = {tr("Unable to analyze the deck on TappedOut.")};
+    static const QRegularExpression errorsRx("<div class=\"alert alert-danger.*?<ul>(.*?)</ul>",
+                                             QRegularExpression::DotMatchesEverythingOption);
+    const auto match = errorsRx.match(data);
+    if (match.hasMatch()) {
+        static const QRegularExpression itemRx("<li>(.*?)</li>", QRegularExpression::DotMatchesEverythingOption);
+        auto items = itemRx.globalMatch(match.captured(1));
+        while (items.hasNext()) {
+            const QString error = QTextDocumentFragment::fromHtml(items.next().captured(1)).toPlainText().simplified();
+            if (!error.isEmpty()) {
+                errorMessageList.append(error);
+            }
+        }
+    }
+    qCWarning(TappedOutInterfaceLog) << "TappedOut analysis failed, HTTP status" << httpStatus;
+    showError(errorMessageList.join('\n'));
 }
 
 void TappedOutInterface::getAnalyzeRequestData(const DeckList &deck, QByteArray &data)
@@ -90,13 +106,9 @@ void TappedOutInterface::getAnalyzeRequestData(const DeckList &deck, QByteArray
     DeckList mainboard, sideboard;
     copyDeckSplitMainAndSide(deck, mainboard, sideboard);
 
-    QUrl params;
-    QUrlQuery urlQuery;
-    urlQuery.addQueryItem("name", deck.getName());
-    urlQuery.addQueryItem("mainboard", mainboard.writeToString_Plain(false, true));
-    urlQuery.addQueryItem("sideboard", sideboard.writeToString_Plain(false, true));
-    params.setQuery(urlQuery);
-    data.append(params.query(QUrl::EncodeReserved).toUtf8());
+    data.append("name=" + QUrl::toPercentEncoding(deck.getName()));
+    data.append("&mainboard=" + QUrl::toPercentEncoding(mainboard.writeToString_Plain(false, true)));
+    data.append("&sideboard=" + QUrl::toPercentEncoding(sideboard.writeToString_Plain(false, true)));
 }
 
 void TappedOutInterface::analyzeDeck(const DeckList &deck)
@@ -117,7 +129,9 @@ void TappedOutInterface::copyDeckSplitMainAndSide(const DeckList &source, DeckLi
 {
     auto copyMainOrSide = [&mainboard, &sideboard](const auto node, const auto card) {
         CardInfoPtr dbCard = CardDatabaseManager::query()->getCardInfo(card->getName());
-        if (!dbCard || dbCard->getIsToken() || node->getName() == DECK_ZONE_MAYBEBOARD) {
+        // Exclude confirmed tokens, but preserve cards missing from the local
+        // database, including newly released cards in an imported deck.
+        if ((dbCard && dbCard->getIsToken()) || node->getName() == DECK_ZONE_MAYBEBOARD) {
             return;
         }
 

cockatrice/src/client/network/interfaces/tapped_out_interface.h

@@ -14,6 +14,7 @@
 inline Q_LOGGING_CATEGORY(TappedOutInterfaceLog, "tapped_out_interface");
 
 class QByteArray;
+class QString;
 class QNetworkAccessManager;
 class QNetworkReply;
 class DeckList;
@@ -29,6 +30,8 @@ class TappedOutInterface : public QObject
     Q_OBJECT
 private:
     QNetworkAccessManager *manager;
+    bool replyHandled = false;
+    void showError(const QString &message);
 
     void copyDeckSplitMainAndSide(const DeckList &source, DeckList &mainboard, DeckList &sideboard);
 private slots: