From 20079dc471439088bd51dd8c84c7a1b94aeb862c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 20 Sep 2026 19:58:09 +0200 Subject: [PATCH] [Client] Make the link-connection gates port-aware and keyboard-safe Second-pass review notes for the shared-deck link flow (Cockatrice#7244): - FlowWidget arrow-key navigation is opt-in via addNavigableWidget, so combo/spin controls on the analytics flows keep their own arrow keys - isConnectedTo and the open-deck/join-game preconditions compare the configured server port alongside the host, so a same-host/different-port link cannot resolve its share token or game id on the wrong instance - the link sign-in dialog reuses an existing server entry's saved name instead of renaming it to the raw hostname - skipStartupAutoConnect is cleared once the launch chain connects, so a later mid-session declined link cannot fire the startup fallback - the plain-launch path of SingleInstanceManager no longer blocks on the primary's ACK - link- and server-supplied text is html-escaped in the confirm prompts and shared-deck preview so markup cannot spoof the shown messages --- .../intents/intent_join_server_game.cpp | 11 ++++++--- .../src/interface/intents/intent_login.cpp | 15 ++++++++++-- .../intents/intent_open_shared_deck.cpp | 13 +++++++---- .../src/interface/intents/url_parser.cpp | 18 ++++++++++----- .../deck_share/shared_deck_preview_widget.cpp | 3 ++- .../dialogs/dlg_shared_decks_preview.cpp | 9 +++++--- .../general/layout_containers/flow_widget.cpp | 23 ++++++++++++++++--- .../general/layout_containers/flow_widget.h | 1 + cockatrice/src/interface/window_main.cpp | 11 ++++++++- cockatrice/src/single_instance_manager.cpp | 8 +++++++ 10 files changed, 89 insertions(+), 23 deletions(-) diff --git a/cockatrice/src/interface/intents/intent_join_server_game.cpp b/cockatrice/src/interface/intents/intent_join_server_game.cpp index 6bf46ec72..b22b200ed 100644 --- a/cockatrice/src/interface/intents/intent_join_server_game.cpp +++ b/cockatrice/src/interface/intents/intent_join_server_game.cpp @@ -19,12 +19,17 @@ bool IntentJoinServerGame::checkPrecondition() const if (remoteClient->getStatus() != ClientStatus::StatusLoggedIn) { return false; } - // serverName() reflects the server the client was configured to connect to, - // which may differ from the actual TCP peer (e.g. when connecting through a - // proxy), so only the hostname is compared here. + // serverName()/serverPort() reflect the server the client was configured + // to connect to, which may differ from the actual TCP peer (e.g. when + // connecting through a proxy), so compare those configured values. A link + // naming the same host on another port is a different server and must not + // reuse the session there. if (remoteClient->serverName().compare(context->roomContext.serverContext.hostname, Qt::CaseInsensitive) != 0) { return false; } + if (QString::number(remoteClient->serverPort()) != context->roomContext.serverContext.port) { + return false; + } if (!tabSupervisor->getRoomTabs().contains(context->roomContext.roomId)) { return false; diff --git a/cockatrice/src/interface/intents/intent_login.cpp b/cockatrice/src/interface/intents/intent_login.cpp index d3431bb07..7beb63e1d 100644 --- a/cockatrice/src/interface/intents/intent_login.cpp +++ b/cockatrice/src/interface/intents/intent_login.cpp @@ -60,8 +60,19 @@ void IntentGetLoginCredentials::onPreconditionNotSatisfied() if (dialog.savePassword() && !context->username.isEmpty()) { ServersSettings &servers = SettingsCache::instance().servers(); - servers.addNewServer(context->hostname, context->hostname, context->port, context->username, context->password, - true); + // The host may already be saved under a friendly name (e.g. a public-server + // list entry) with no credentials; reuse that name instead of overwriting + // it with the raw hostname when addNewServer updates the entry in place. + QString saveName = context->hostname; + const int existingIndex = servers.findServerIndex(context->hostname, context->port); + if (existingIndex >= 0) { + saveName = + servers.getValue(QString("saveName%1").arg(existingIndex), "server", "server_details").toString(); + if (saveName.isEmpty()) { + saveName = context->hostname; + } + } + servers.addNewServer(saveName, context->hostname, context->port, context->username, context->password, true); } emitFinished(); diff --git a/cockatrice/src/interface/intents/intent_open_shared_deck.cpp b/cockatrice/src/interface/intents/intent_open_shared_deck.cpp index 9ecb4520d..016de63e6 100644 --- a/cockatrice/src/interface/intents/intent_open_shared_deck.cpp +++ b/cockatrice/src/interface/intents/intent_open_shared_deck.cpp @@ -34,10 +34,15 @@ bool IntentOpenSharedDeck::checkPrecondition() const if (remoteClient->getStatus() != ClientStatus::StatusLoggedIn) { return false; } - // serverName() reflects the server the client was configured to connect to, - // which may differ from the actual TCP peer (e.g. when connecting through a - // proxy), so only the hostname is compared here. - return remoteClient->serverName().compare(context->serverContext.hostname, Qt::CaseInsensitive) == 0; + // serverName()/serverPort() reflect the server the client was configured + // to connect to, which may differ from the actual TCP peer (e.g. when + // connecting through a proxy), so compare those configured values. The + // share token must be resolved against the host the link named — a link to + // the same host on another port is a different server. + if (remoteClient->serverName().compare(context->serverContext.hostname, Qt::CaseInsensitive) != 0) { + return false; + } + return QString::number(remoteClient->serverPort()) == context->serverContext.port; } void IntentOpenSharedDeck::onPreconditionSatisfied() diff --git a/cockatrice/src/interface/intents/url_parser.cpp b/cockatrice/src/interface/intents/url_parser.cpp index 120e72b2a..707863354 100644 --- a/cockatrice/src/interface/intents/url_parser.cpp +++ b/cockatrice/src/interface/intents/url_parser.cpp @@ -208,9 +208,13 @@ Intent *IntentUrlParser::createOpenDeckIntent(const QUrlQuery &query, PendingInt // taking the session anywhere it isn't already, naming the host we would // connect to. Remember the link's target when it moves us away from a live // session so a failed or cancelled chain can restore the session it left. + // The hostname is link-supplied and percent-decoded, so escape it: QMessageBox + // renders AutoText, and markup in a hostname would otherwise flip the whole + // prompt to rich text and let a link pad the message the user is shown. const bool alreadyConnected = isConnectedTo(ctx->serverContext.hostname, ctx->serverContext.port); if (!alreadyConnected) { - const QString target = QStringLiteral("%1:%2").arg(ctx->serverContext.hostname, ctx->serverContext.port); + const QString target = + QStringLiteral("%1:%2").arg(ctx->serverContext.hostname.toHtmlEscaped(), ctx->serverContext.port); if (client->getStatus() == StatusLoggedIn) { const QString current = @@ -273,12 +277,14 @@ Intent *IntentUrlParser::createOpenDeckIntent(const QUrlQuery &query, PendingInt bool IntentUrlParser::isConnectedTo(const QString &hostname, const QString &port) const { - Q_UNUSED(port); - // Deliberately hostname-only (no port): the intents' preconditions apply the - // same rule, so a link to the same host on another port still connects - // rather than silently reusing an existing session on a different server. + // serverName() reflects the server the client was configured to connect to, + // which may differ from the actual TCP peer (e.g. when connecting through a + // proxy), so compare the configured host and port — exactly what a link + // names. A link to the same host on another port is a different server and + // must not silently reuse an existing session there. RemoteClient *client = mainWindow->getRemoteClient(); - return client->getStatus() == StatusLoggedIn && client->serverName().compare(hostname, Qt::CaseInsensitive) == 0; + return client->getStatus() == StatusLoggedIn && client->serverName().compare(hostname, Qt::CaseInsensitive) == 0 && + QString::number(client->serverPort()) == port; } void IntentUrlParser::startNextChain() diff --git a/cockatrice/src/interface/widgets/deck_share/shared_deck_preview_widget.cpp b/cockatrice/src/interface/widgets/deck_share/shared_deck_preview_widget.cpp index ef77df9ec..21ec80e08 100644 --- a/cockatrice/src/interface/widgets/deck_share/shared_deck_preview_widget.cpp +++ b/cockatrice/src/interface/widgets/deck_share/shared_deck_preview_widget.cpp @@ -33,7 +33,8 @@ SharedDeckPreviewWidget::SharedDeckPreviewWidget(QWidget *parent, colorIdentityWidget = new ColorIdentityWidget(this, colorIdentity); colorIdentityWidget->setVisible(!colorIdentity.isEmpty()); - gameFormatLabel = new QLabel(gameFormat, this); + // gameFormat is server-supplied and the QLabel renders AutoText, so escape it. + gameFormatLabel = new QLabel(gameFormat.toHtmlEscaped(), this); gameFormatLabel->setAlignment(Qt::AlignCenter); gameFormatLabel->setVisible(!gameFormat.isEmpty()); diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_shared_decks_preview.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_shared_decks_preview.cpp index e84602cd6..cd7ecdcdd 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_shared_decks_preview.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_shared_decks_preview.cpp @@ -25,7 +25,10 @@ DlgSharedDecksPreview::DlgSharedDecksPreview(QWidget *parent, auto *mainLayout = new QVBoxLayout(this); - auto *titleLabel = new QLabel(tr("Share: %1").arg(shareName.isEmpty() ? tr("Untitled") : shareName), this); + // shareName and serverText come from the share server, so escape them: the + // QLabels render AutoText and markup would otherwise be shown as rich text. + auto *titleLabel = + new QLabel(tr("Share: %1").arg((shareName.isEmpty() ? tr("Untitled") : shareName).toHtmlEscaped()), this); QFont titleFont = titleLabel->font(); titleFont.setBold(true); titleFont.setPointSize(titleFont.pointSize() + 2); @@ -33,7 +36,7 @@ DlgSharedDecksPreview::DlgSharedDecksPreview(QWidget *parent, mainLayout->addWidget(titleLabel); if (!serverText.isEmpty()) { - mainLayout->addWidget(new QLabel(tr("From %1").arg(serverText), this)); + mainLayout->addWidget(new QLabel(tr("From %1").arg(serverText.toHtmlEscaped()), this)); } if (expiresAt > 0) { @@ -57,7 +60,7 @@ DlgSharedDecksPreview::DlgSharedDecksPreview(QWidget *parent, auto *tile = new SharedDeckPreviewWidget( this, querier, QString::fromStdString(item.name()), QString::fromStdString(item.banner_card()), QString::fromStdString(item.color_identity()), QString::fromStdString(item.game_format()), tags.join(", ")); - flowWidget->addWidget(tile); + flowWidget->addNavigableWidget(tile); tiles.append(tile); itemIds.append(item.id()); } diff --git a/cockatrice/src/interface/widgets/general/layout_containers/flow_widget.cpp b/cockatrice/src/interface/widgets/general/layout_containers/flow_widget.cpp index 05dcf3e8e..01c9ac34e 100644 --- a/cockatrice/src/interface/widgets/general/layout_containers/flow_widget.cpp +++ b/cockatrice/src/interface/widgets/general/layout_containers/flow_widget.cpp @@ -81,13 +81,30 @@ FlowWidget::FlowWidget(QWidget *parent, /** * @brief Adds a widget to the flow layout within the FlowWidget. * - * The widget is filtered for arrow-key events so keyboard navigation between - * the flow items keeps working even when the flow sits inside a QScrollArea, - * which swallows arrow keys before they can reach FlowWidget::keyPressEvent. + * Plain widgets are not filtered for arrow keys: intercepting them would steal + * Up/Down/Left/Right from controls that use them (combo boxes, spin boxes + * etc.). Widgets that want keyboard navigation between flow items must be + * added via addNavigableWidget instead. * * @param widget_to_add The widget to add to the flow layout. */ void FlowWidget::addWidget(QWidget *widget_to_add) +{ + flowLayout->addWidget(widget_to_add); +} + +/** + * @brief Adds a widget and routes its arrow keys to FlowWidget focus navigation. + * + * The widget is filtered for arrow-key events so keyboard navigation between + * the flow items keeps working even when the flow sits inside a QScrollArea, + * which swallows arrow keys before they can reach FlowWidget::keyPressEvent. + * Only widgets added through this method are affected; anything that needs its + * own arrow keys should use plain addWidget. + * + * @param widget_to_add The widget to add to the flow layout. + */ +void FlowWidget::addNavigableWidget(QWidget *widget_to_add) { widget_to_add->installEventFilter(this); flowLayout->addWidget(widget_to_add); diff --git a/cockatrice/src/interface/widgets/general/layout_containers/flow_widget.h b/cockatrice/src/interface/widgets/general/layout_containers/flow_widget.h index bdcb1e590..4d52db3f1 100644 --- a/cockatrice/src/interface/widgets/general/layout_containers/flow_widget.h +++ b/cockatrice/src/interface/widgets/general/layout_containers/flow_widget.h @@ -30,6 +30,7 @@ public: Qt::ScrollBarPolicy verticalPolicy); void addWidget(QWidget *widget_to_add); + void addNavigableWidget(QWidget *widget_to_add); void insertWidgetAtIndex(QWidget *toInsert, int index); void removeWidget(QWidget *widgetToRemove) const; void clearLayout(); diff --git a/cockatrice/src/interface/window_main.cpp b/cockatrice/src/interface/window_main.cpp index b49146101..595d38d5b 100644 --- a/cockatrice/src/interface/window_main.cpp +++ b/cockatrice/src/interface/window_main.cpp @@ -939,7 +939,16 @@ void MainWindow::onUrlChainFinished(bool connected) // chain ended without connecting (declined, invalid, offline), fall back to // the startup connection so the activation launch still behaves like a // normal launch. - if (connected || !skipStartupAutoConnect || getRemoteClient()->getStatus() != StatusDisconnected) { + if (connected) { + // The launch link connected, so the startup fallback has served its + // purpose: drop the skip so a later mid-session link that ends declined + // or offline cannot silently fire auto-connect or applyStartupDestination + // again. + skipStartupAutoConnect = false; + return; + } + + if (!skipStartupAutoConnect || getRemoteClient()->getStatus() != StatusDisconnected) { return; } diff --git a/cockatrice/src/single_instance_manager.cpp b/cockatrice/src/single_instance_manager.cpp index a0db7faf6..eff0e6586 100644 --- a/cockatrice/src/single_instance_manager.cpp +++ b/cockatrice/src/single_instance_manager.cpp @@ -92,6 +92,14 @@ SingleInstanceManager::ForwardResult SingleInstanceManager::forwardToPrimary(con socket.flush(); socket.waitForBytesWritten(1000); + // A plain launch has nothing for the primary to act on, so there is nothing + // to acknowledge. Waiting here would block the new instance for seconds if + // the primary is busy in a modal dialog, so only the activation path (which + // needs the ACK to avoid stealing a live primary's socket) waits below. + if (filesToSend.isEmpty()) { + return ForwardResult::Delivered; + } + // Only report a successful hand-off once the primary has acknowledged that // it actually read the payload. A socket that connects but is still working // on an earlier payload is alive but busy, not dead: give it more room