[DeckSync] Match server rows by identity and hash the whole deck

Sync keyed local files to server rows by path, but the server names a row
after the deck, not after the file, so any deck whose file name and deck name
differ uploaded again on every run. A second pass then brought the server copy
back as an extra local file.

- Match by the sidecar's row id, then guid, then content, and only then by the
  server path, so renaming either side no longer duplicates a deck.
- Key the server tree by row id: the server allows several rows with the same
  name in one folder, and keying by path silently dropped all but one.
- Stop discarding rows whose name contains '/' ("U/W Control"). They stay
  matchable, and only the download file name is sanitised.
- Build the download path from the folder plus a sanitised row name, including
  every folder segment, so a hostile server name cannot escape the deck folder.
- Download a colliding server row under a unique name and count it, instead of
  skipping it without a trace.
- Hash the whole deck instead of the card hash: SHA-1 over the native deck
  string covers name, printings, banner, tokens and sideboard plans. The client
  and the server hash the exact bytes they exchange, so both agree.

Adds deck_content_hash_test for the client/server hashing invariant and for the
name-only change the card hash cannot see.
This commit is contained in:
Lukas Brübach 2026-11-01 12:43:02 +01:00
parent ad0da98a86
commit 2916fa45af
7 changed files with 215 additions and 81 deletions

View file

@ -7,6 +7,7 @@
#include <QDirIterator> #include <QDirIterator>
#include <QFile> #include <QFile>
#include <QFileInfo> #include <QFileInfo>
#include <QStringList>
#include <QUuid> #include <QUuid>
#include <libcockatrice/deck_list/deck_file_format.h> #include <libcockatrice/deck_list/deck_file_format.h>
#include <libcockatrice/deck_list/deck_list.h> #include <libcockatrice/deck_list/deck_list.h>
@ -36,6 +37,35 @@ QString serverPathFor(const QString &relativePath)
} }
return path; return path;
} }
// Rows are named after the deck, not after the file, so a row name may contain
// characters a file name cannot ("U/W Control"). Downloads therefore rebuild the
// local path from the folder plus a sanitised row name, and never from the raw
// server-supplied path.
QString sanitizedSegment(const QString &segment)
{
QString safe = segment;
safe.replace('/', '-');
safe.replace('\\', '-');
safe = safe.trimmed();
if (safe.isEmpty() || safe == QStringLiteral(".") || safe == QStringLiteral("..")) {
return QStringLiteral("deck");
}
return safe;
}
QString localPathFor(const QString &dir, const QString &name)
{
// Every segment is server-supplied, so each one is sanitised before it can
// become part of a local path.
QStringList segments;
for (const QString &segment : dir.split('/', Qt::SkipEmptyParts)) {
segments << sanitizedSegment(segment);
}
segments << sanitizedSegment(name);
const QString path = segments.join('/');
return path.endsWith(".cod", Qt::CaseInsensitive) ? path : path + QStringLiteral(".cod");
}
} // namespace } // namespace
DeckSyncManager::DeckSyncManager(QObject *parent) : QObject(parent) DeckSyncManager::DeckSyncManager(QObject *parent) : QObject(parent)
@ -111,7 +141,7 @@ void DeckSyncManager::syncNow()
continue; continue;
} }
localFiles.insert(relativePath, LocalFile{.absolutePath = absolutePath, localFiles.insert(relativePath, LocalFile{.absolutePath = absolutePath,
.hash = loaded->deckList.getDeckHash(), .hash = loaded->deckList.getContentHash(),
.modified = QFileInfo(absolutePath).lastModified()}); .modified = QFileInfo(absolutePath).lastModified()});
} }
@ -123,28 +153,30 @@ void DeckSyncManager::syncNow()
void DeckSyncManager::collectServerFiles(const ServerInfo_DeckStorage_Folder &folder, void DeckSyncManager::collectServerFiles(const ServerInfo_DeckStorage_Folder &folder,
const QString &prefix, const QString &prefix,
QMap<QString, ServerFile> &files, QMap<int, ServerFile> &files,
QSet<QString> &dirs) QSet<QString> &dirs)
{ {
for (const auto &item : folder.items()) { for (const auto &item : folder.items()) {
const QString name = QString::fromStdString(item.name()); const QString name = QString::fromStdString(item.name());
if (name.isEmpty() || name == "." || name == ".." || name.contains('/') || name.contains('\\')) { const QString path = prefix.isEmpty() ? name : prefix + "/" + name;
continue;
}
if (item.has_folder()) { if (item.has_folder()) {
const QString path = prefix.isEmpty() ? name : prefix + "/" + name;
dirs.insert(path); dirs.insert(path);
collectServerFiles(item.folder(), path, files, dirs); collectServerFiles(item.folder(), path, files, dirs);
} else if (item.has_file()) { } else if (item.has_file()) {
const QString path = prefix.isEmpty() ? name : prefix + "/" + name;
const ServerInfo_DeckStorage_File &file = item.file(); const ServerInfo_DeckStorage_File &file = item.file();
const int id = static_cast<int>(item.id());
ServerFile entry{ ServerFile entry{
.id = static_cast<int>(item.id()), .id = id,
.path = path,
.dir = prefix,
.name = name,
.guid = QString::fromStdString(file.guid()), .guid = QString::fromStdString(file.guid()),
.hash = QString::fromStdString(file.hash()), .hash = QString::fromStdString(file.hash()),
.lastModifiedSecs = file.has_last_modified() ? static_cast<qint64>(file.last_modified()) : 0, .lastModifiedSecs = file.has_last_modified() ? static_cast<qint64>(file.last_modified()) : 0,
}; };
files.insert(path, entry); // Keyed by id: the server allows several rows with the same name in a
// folder, and keying by path silently dropped all but one of them.
files.insert(id, entry);
} }
} }
} }
@ -159,7 +191,7 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont
return; return;
} }
QMap<QString, ServerFile> serverFiles; QMap<int, ServerFile> serverFiles;
QSet<QString> serverDirs; QSet<QString> serverDirs;
const Response_DeckList &list = response.GetExtension(Response_DeckList::ext); const Response_DeckList &list = response.GetExtension(Response_DeckList::ext);
collectServerFiles(list.root(), QString(), serverFiles, serverDirs); collectServerFiles(list.root(), QString(), serverFiles, serverDirs);
@ -169,57 +201,78 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont
DeckSyncSidecar sidecar(sidecarPath); DeckSyncSidecar sidecar(sidecarPath);
sidecar.load(); sidecar.load();
const qint64 nowSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); const qint64 nowSecs = QDateTime::currentDateTime().toSecsSinceEpoch();
// Reuse the sidecar's guid so the server keeps recognizing the row, and mint
// one only for a deck that has never been synced.
auto guidFor = [&sidecar](const QString &relativePath) {
const DeckSyncRecord record = sidecar.get(relativePath);
return record.guid.isEmpty() ? QUuid::createUuid().toString(QUuid::WithoutBraces) : record.guid;
};
QMap<int, QString> serverPathById; QMap<QString, int> serverIdByPath;
QMap<QString, QString> serverPathByGuid; QMap<QString, int> serverIdByGuid;
for (auto sit = serverFiles.begin(); sit != serverFiles.end(); ++sit) { for (auto sit = serverFiles.begin(); sit != serverFiles.end(); ++sit) {
serverPathById.insert(sit.value().id, sit.key()); serverIdByPath.insert(sit.value().path, sit.key());
if (!sit.value().guid.isEmpty()) { if (!sit.value().guid.isEmpty()) {
serverPathByGuid.insert(sit.value().guid, sit.key()); serverIdByGuid.insert(sit.value().guid, sit.key());
} }
} }
QSet<QString> matchedServerPaths; QSet<int> matchedServerIds;
// A local file without a usable sidecar record still usually has a server
// counterpart: rows are named after the deck, so compare content rather than
// file names, and only upload when no unmatched row has the same content.
auto unmatchedByHash = [&serverFiles, &matchedServerIds](const QString &hash) {
if (hash.isEmpty()) {
return -1;
}
for (auto sit = serverFiles.begin(); sit != serverFiles.end(); ++sit) {
if (!matchedServerIds.contains(sit.key()) && sit.value().hash == hash) {
return sit.key();
}
}
return -1;
};
for (auto it = localFiles.begin(); it != localFiles.end(); ++it) { for (auto it = localFiles.begin(); it != localFiles.end(); ++it) {
const DeckSyncRecord sidecarRecord = sidecar.get(it.key()); const DeckSyncRecord sidecarRecord = sidecar.get(it.key());
QString serverPath; int serverId = -1;
if (sidecarRecord.remoteId != -1 && serverPathById.contains(sidecarRecord.remoteId)) { if (sidecarRecord.remoteId != -1 && serverFiles.contains(sidecarRecord.remoteId)) {
serverPath = serverPathById.value(sidecarRecord.remoteId); serverId = sidecarRecord.remoteId;
} else if (!sidecarRecord.guid.isEmpty() && serverPathByGuid.contains(sidecarRecord.guid)) { } else if (!sidecarRecord.guid.isEmpty() && serverIdByGuid.contains(sidecarRecord.guid)) {
serverPath = serverPathByGuid.value(sidecarRecord.guid); serverId = serverIdByGuid.value(sidecarRecord.guid);
} else { } else {
serverPath = serverPathFor(it.key()); // Content first: a row with the same deck is unambiguous, while a
if (!serverFiles.contains(serverPath)) { // matching file name may belong to a different deck entirely.
UploadOp op; serverId = unmatchedByHash(it.value().hash);
op.relativePath = it.key(); if (serverId == -1) {
op.serverPath = serverPath; const QString wantedPath = serverPathFor(it.key());
op.deckId = -1; if (serverIdByPath.contains(wantedPath)) {
op.guid = sidecar.contains(it.key()) && !sidecar.get(it.key()).guid.isEmpty() const int pathId = serverIdByPath.value(wantedPath);
? sidecar.get(it.key()).guid if (!matchedServerIds.contains(pathId)) {
: QUuid::createUuid().toString(QUuid::WithoutBraces); serverId = pathId;
pendingUploads.append(op); }
continue; }
} }
} }
auto serverIt = serverFiles.find(serverPath);
if (serverIt == serverFiles.end()) { if (serverId == -1) {
UploadOp op; const QString serverPath = serverPathFor(it.key());
op.relativePath = it.key(); pendingUploads.append(UploadOp{.relativePath = it.key(),
op.serverPath = serverPath; .serverPath = serverPath,
op.deckId = -1; .serverDir = parentDir(serverPath),
op.guid = sidecar.contains(it.key()) && !sidecar.get(it.key()).guid.isEmpty() .deckId = -1,
? sidecar.get(it.key()).guid .guid = guidFor(it.key())});
: QUuid::createUuid().toString(QUuid::WithoutBraces);
pendingUploads.append(op);
continue; continue;
} }
matchedServerPaths.insert(serverIt.key());
if (it.value().hash == serverIt.value().hash) { const ServerFile &server = serverFiles.value(serverId);
matchedServerIds.insert(serverId);
if (it.value().hash == server.hash) {
DeckSyncRecord record = sidecar.get(it.key()); DeckSyncRecord record = sidecar.get(it.key());
if (record.guid.isEmpty()) { if (record.guid.isEmpty()) {
record.guid = serverIt.value().guid; record.guid = server.guid;
} }
record.remoteId = serverIt.value().id; record.remoteId = server.id;
record.baseHash = it.value().hash; record.baseHash = it.value().hash;
record.lastSyncSecs = nowSecs; record.lastSyncSecs = nowSecs;
record.deleted = false; record.deleted = false;
@ -230,44 +283,36 @@ void DeckSyncManager::onListFinished(const Response &response, const CommandCont
const qint64 localSecs = it.value().modified.toSecsSinceEpoch(); const qint64 localSecs = it.value().modified.toSecsSinceEpoch();
// A server row without last_modified predates sync: the local file wins. // A server row without last_modified predates sync: the local file wins.
// Otherwise strictly newer wins; ties go to the server. // Otherwise strictly newer wins; ties go to the server.
if (serverIt.value().lastModifiedSecs == 0 || localSecs > serverIt.value().lastModifiedSecs) { if (server.lastModifiedSecs == 0 || localSecs > server.lastModifiedSecs) {
UploadOp op; pendingUploads.append(UploadOp{.relativePath = it.key(),
op.relativePath = it.key(); .serverPath = server.path,
op.serverPath = serverPath; .serverDir = server.dir,
op.deckId = serverIt.value().id; .deckId = server.id,
op.guid = serverIt.value().guid; .guid = server.guid});
pendingUploads.append(op);
} else { } else {
DownloadOp op; pendingDownloads.append(DownloadOp{.relativePath = it.key(),
op.relativePath = it.key(); .deckId = server.id,
op.deckId = serverIt.value().id; .guid = server.guid,
op.guid = serverIt.value().guid; .hash = server.hash,
op.hash = serverIt.value().hash; .lastModifiedSecs = server.lastModifiedSecs});
op.lastModifiedSecs = serverIt.value().lastModifiedSecs;
pendingDownloads.append(op);
} }
} }
for (auto it = serverFiles.begin(); it != serverFiles.end(); ++it) { for (auto it = serverFiles.begin(); it != serverFiles.end(); ++it) {
if (matchedServerPaths.contains(it.key())) { if (matchedServerIds.contains(it.key())) {
continue;
}
const QString wantedPath = it.key().endsWith(".cod", Qt::CaseInsensitive) ? it.key() : it.key() + ".cod";
if (localFiles.contains(wantedPath) || QFileInfo(deckPath + "/" + wantedPath).exists()) {
continue; continue;
} }
const QString wantedPath = localPathFor(it.value().dir, it.value().name);
const QString relativePath = uniqueLocalPath(wantedPath); const QString relativePath = uniqueLocalPath(wantedPath);
if (relativePath.isEmpty()) { if (relativePath.isEmpty()) {
++skippedCount; ++skippedCount;
continue; continue;
} }
DownloadOp op; pendingDownloads.append(DownloadOp{.relativePath = relativePath,
op.relativePath = relativePath; .deckId = it.value().id,
op.deckId = it.value().id; .guid = it.value().guid,
op.guid = it.value().guid; .hash = it.value().hash,
op.hash = it.value().hash; .lastModifiedSecs = it.value().lastModifiedSecs});
op.lastModifiedSecs = it.value().lastModifiedSecs;
pendingDownloads.append(op);
} }
sidecar.save(); sidecar.save();
@ -297,7 +342,7 @@ void DeckSyncManager::processNext()
void DeckSyncManager::processNextUpload() void DeckSyncManager::processNextUpload()
{ {
const UploadOp op = pendingUploads.first(); const UploadOp op = pendingUploads.first();
const QString dir = parentDir(op.serverPath); const QString dir = op.serverDir;
if (!ensuredDirs.contains(dir)) { if (!ensuredDirs.contains(dir)) {
sendMkDirFor(dir); sendMkDirFor(dir);
return; return;
@ -370,7 +415,7 @@ void DeckSyncManager::sendUpload(const UploadOp &op)
Command_DeckUpload cmd; Command_DeckUpload cmd;
if (op.deckId == -1) { if (op.deckId == -1) {
cmd.set_path(parentDir(op.serverPath).toStdString()); cmd.set_path(op.serverDir.toStdString());
} else { } else {
cmd.set_deck_id(op.deckId); cmd.set_deck_id(op.deckId);
} }
@ -416,7 +461,7 @@ void DeckSyncManager::onUploadFinished(const Response &response,
} else { } else {
record.lastSyncSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); record.lastSyncSecs = QDateTime::currentDateTime().toSecsSinceEpoch();
} }
record.remoteId = file.id(); record.remoteId = static_cast<int>(file.id());
} else if (localFiles.contains(relativePath)) { } else if (localFiles.contains(relativePath)) {
record.baseHash = localFiles.value(relativePath).hash; record.baseHash = localFiles.value(relativePath).hash;
record.lastSyncSecs = QDateTime::currentDateTime().toSecsSinceEpoch(); record.lastSyncSecs = QDateTime::currentDateTime().toSecsSinceEpoch();
@ -517,7 +562,7 @@ void DeckSyncManager::onDownloadFinished(const Response &response,
if (!download.hash().empty()) { if (!download.hash().empty()) {
record.baseHash = QString::fromStdString(download.hash()); record.baseHash = QString::fromStdString(download.hash());
} else { } else {
record.baseHash = deck.getDeckHash(); record.baseHash = deck.getContentHash();
} }
record.lastSyncSecs = download.has_last_modified() ? static_cast<qint64>(download.last_modified()) record.lastSyncSecs = download.has_last_modified() ? static_cast<qint64>(download.last_modified())
: QDateTime::currentDateTime().toSecsSinceEpoch(); : QDateTime::currentDateTime().toSecsSinceEpoch();

View file

@ -30,9 +30,11 @@ class ServerInfo_DeckStorage_Folder;
* Each syncNow() run loads the profile sidecar, scans the deck folder, fetches * Each syncNow() run loads the profile sidecar, scans the deck folder, fetches
* the server tree and mirrors both sides: unknown local decks upload, unknown * the server tree and mirrors both sides: unknown local decks upload, unknown
* server decks download, and decks changed on both sides resolve newer-wins * server decks download, and decks changed on both sides resolve newer-wins
* (ties go to the server). Matching is path-based; a rename looks like a * (ties go to the server). A local file is matched to its server row by the
* delete plus an add. Before a download overwrites a locally modified file, the * sidecar's row id, then guid, then the server path, and finally by content, so
* local content is copied to a .sync-backup file next to it. * renaming either side does not duplicate a deck. Before a download overwrites a
* locally modified file, the local content is copied to a .sync-backup file next
* to it.
* *
* Ownership: fire-and-forget per run, driven by PendingCommand round trips. * Ownership: fire-and-forget per run, driven by PendingCommand round trips.
* No watcher, no auto triggers, no conflict UI — those arrive in later layers. * No watcher, no auto triggers, no conflict UI — those arrive in later layers.
@ -81,6 +83,9 @@ private:
struct ServerFile struct ServerFile
{ {
int id = -1; int id = -1;
QString path; ///< Server-side address: folder prefix plus row name.
QString dir; ///< Folder prefix, empty in the root.
QString name; ///< Row name exactly as the server stores it.
QString guid; QString guid;
QString hash; QString hash;
qint64 lastModifiedSecs = 0; qint64 lastModifiedSecs = 0;
@ -94,7 +99,8 @@ private:
struct UploadOp struct UploadOp
{ {
QString relativePath; QString relativePath;
QString serverPath; QString serverPath; ///< Target address of a brand-new row.
QString serverDir; ///< Folder that must exist before the upload.
int deckId = -1; int deckId = -1;
QString guid; QString guid;
}; };
@ -112,7 +118,7 @@ private:
[[nodiscard]] SyncSummary currentSummary() const; [[nodiscard]] SyncSummary currentSummary() const;
void collectServerFiles(const ServerInfo_DeckStorage_Folder &folder, void collectServerFiles(const ServerInfo_DeckStorage_Folder &folder,
const QString &prefix, const QString &prefix,
QMap<QString, ServerFile> &files, QMap<int, ServerFile> &files,
QSet<QString> &dirs); QSet<QString> &dirs);
void processNext(); void processNext();
void processNextUpload(); void processNextUpload();

View file

@ -9,6 +9,7 @@
#include "tree/inner_deck_list_node.h" #include "tree/inner_deck_list_node.h"
#include <QAnyStringView> #include <QAnyStringView>
#include <QCryptographicHash>
#include <QDebug> #include <QDebug>
#include <QSet> #include <QSet>
#include <QSharedPointer> #include <QSharedPointer>
@ -453,6 +454,16 @@ void DeckList::refreshDeckHash()
cachedDeckHash = QString(); cachedDeckHash = QString();
} }
QString DeckList::contentHash(const QString &nativeDeck)
{
return QString::fromLatin1(QCryptographicHash::hash(nativeDeck.toUtf8(), QCryptographicHash::Sha1).toHex());
}
QString DeckList::getContentHash() const
{
return contentHash(writeToString_Native());
}
/** /**
* Calls a given function on each card in the deck. * Calls a given function on each card in the deck.
*/ */

View file

@ -289,6 +289,16 @@ public:
/** @name Deck identity */ /** @name Deck identity */
///@{ ///@{
QString getDeckHash() const; QString getDeckHash() const;
/**
* @brief SHA-1 of a native deck string, covering everything the card hash
* ignores (name, printings, banner, tokens, comments, sideboard plans).
*
* Deck-collection sync hashes the exact string it sends or receives, so both
* peers compare the same bytes.
*/
static QString contentHash(const QString &nativeDeck);
/** @brief contentHash() of this deck's native serialisation. */
QString getContentHash() const;
void refreshDeckHash(); void refreshDeckHash();
///@} ///@}

View file

@ -1114,7 +1114,9 @@ Response::ResponseCode AbstractServerSocketInterface::cmdDeckUpload(const Comman
const QString bannerCardProvider = deck.getBannerCard().providerId.left(32); const QString bannerCardProvider = deck.getBannerCard().providerId.left(32);
const QString tagsJson = serializeDeckTags(deck.getTags()); const QString tagsJson = serializeDeckTags(deck.getTags());
const QString colorIdentity = sanitizeColorIdentity(nameFromStdString(cmd.color_identity())); const QString colorIdentity = sanitizeColorIdentity(nameFromStdString(cmd.color_identity()));
const QString deckHash = deck.getDeckHash(); // Hash the exact bytes the client sent: both peers must compare the same
// string, not each side's re-serialisation of it.
const QString deckHash = DeckList::contentHash(deckStr);
QString guid = cmd.has_guid() ? nameFromStdString(cmd.guid()).left(36) : QString(); QString guid = cmd.has_guid() ? nameFromStdString(cmd.guid()).left(36) : QString();
if (QUuid::fromString(guid).isNull()) { if (QUuid::fromString(guid).isNull()) {
guid = QUuid::createUuid().toString(QUuid::WithoutBraces); guid = QUuid::createUuid().toString(QUuid::WithoutBraces);

View file

@ -416,6 +416,15 @@ cockatrice_add_test(
libcockatrice_deck_list libcockatrice_deck_list
libcockatrice_utility libcockatrice_utility
) )
cockatrice_add_test(
NAME
deck_content_hash_test
SOURCES
deck_content_hash_test.cpp
LIBS
libcockatrice_deck_list
libcockatrice_utility
)
cockatrice_add_test( cockatrice_add_test(
NAME NAME
deck_sync_profile_test deck_sync_profile_test

View file

@ -0,0 +1,51 @@
#include "gtest/gtest.h"
#include <QString>
#include <libcockatrice/deck_list/deck_list.h>
namespace
{
QString deckWithName(const QString &name)
{
return QStringLiteral(R"(<?xml version="1.0"?><cockatrice_deck version="1">)"
R"(<deckname>%1</deckname><comments></comments>)"
R"(<zone name="main"><card number="4" name="Lightning Bolt"/></zone>)"
R"(<zone name="side"><card number="2" name="Pyroblast"/></zone>)"
R"(</cockatrice_deck>)")
.arg(name);
}
} // namespace
TEST(DeckContentHashTest, MatchesNativeSerialisation)
{
DeckList deck;
ASSERT_TRUE(deck.loadFromString_Native(deckWithName(QStringLiteral("Burn"))));
EXPECT_EQ(deck.getContentHash(), DeckList::contentHash(deck.writeToString_Native()));
}
TEST(DeckContentHashTest, IsStableAcrossLoads)
{
DeckList first;
DeckList second;
ASSERT_TRUE(first.loadFromString_Native(deckWithName(QStringLiteral("Burn"))));
ASSERT_TRUE(second.loadFromString_Native(deckWithName(QStringLiteral("Burn"))));
EXPECT_EQ(first.getContentHash(), second.getContentHash());
}
TEST(DeckContentHashTest, CoversDeckName)
{
DeckList burn;
DeckList renamed;
ASSERT_TRUE(burn.loadFromString_Native(deckWithName(QStringLiteral("Burn"))));
ASSERT_TRUE(renamed.loadFromString_Native(deckWithName(QStringLiteral("Burn v2"))));
// The card hash only looks at card names and counts, so it cannot tell the
// two decks apart; the content hash must.
EXPECT_EQ(burn.getDeckHash(), renamed.getDeckHash());
EXPECT_NE(burn.getContentHash(), renamed.getContentHash());
}
int main(int argc, char **argv)
{
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
}