From 69b421f1c2aebe81f071b3a8e9ee8010a7b33921 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sun, 18 Oct 2026 08:26:05 +0200 Subject: [PATCH] [CrashReporting] Log where crash reports go and warn when they cannot The client resolved COCKATRICE_SENTRY_DSN (environment over the baked-in value) without ever saying which one won, and sentry-native refuses to build a request for a DSN it considers invalid without reporting anything, so a tester pointing the client at a scratch project had nothing to go on when a crash never appeared. Say which DSN source won, and log the destination (host and project id, never the key) once the SDK is armed. Warn instead of staying quiet when no DSN is configured at all, when a DSN is malformed - disabling reporting rather than silently falling back to the project baked into the build, which is the rule the installer already follows in cmake/CrashReporting.cmake - and when the build was compiled without Sentry support. A deliberate Never-send stays at info level: that is a choice, not a misconfiguration. Also warn when a pending crash envelope was captured for a different destination than the session that will upload it. sentry-native posts every envelope with the DSN held in the transport state of the session doing the sending, and envelopes restored from disk are raw, so the "dsn" header of a cached envelope is never read: a crash captured for one project can be uploaded to another with nothing in the log to show it. Tests cover the DSN shape check (same unanchored pattern the build matches, so a DSN this build accepted is never rejected here), destination description without the key, and the mismatch detection, which stays quiet for a different key on the same project, unreadable files, and headers without a dsn. --- .../src/client/crash/crash_reporter.cpp | 84 +++++++++++++++++++ cockatrice/src/client/crash/crash_reporter.h | 30 +++++++ cockatrice/src/main.cpp | 22 ++++- tests/crash_reporter_test.cpp | 62 ++++++++++++++ 4 files changed, 197 insertions(+), 1 deletion(-) diff --git a/cockatrice/src/client/crash/crash_reporter.cpp b/cockatrice/src/client/crash/crash_reporter.cpp index 8cf6f633f..720bcd350 100644 --- a/cockatrice/src/client/crash/crash_reporter.cpp +++ b/cockatrice/src/client/crash/crash_reporter.cpp @@ -7,6 +7,8 @@ #include #include #include +#include +#include #include #include #include @@ -151,6 +153,67 @@ sentry_value_t scrubEventBeforeSend(sentry_value_t event, sentry_hint_t *hint, v } // namespace #endif +namespace +{ + +/** + * @brief DSN matcher shared by isDsnWellFormed() and describeDsn(). + * + * Kept unanchored on purpose, mirroring the MATCHES in cmake/CrashReporting.cmake (CMake searches + * rather than anchoring), so the client never rejects a DSN the build itself baked in. Compiled once + * because QRegularExpression construction is not free and both helpers are called per pending + * envelope. + */ +const QRegularExpression &dsnPattern() +{ + static const QRegularExpression pattern(QStringLiteral("(https?)://([^@]+)@([^/]+)/([0-9]+)")); + return pattern; +} + +} // namespace + +bool CrashReporter::isDsnWellFormed(const QString &dsn) +{ + return dsnPattern().match(dsn).hasMatch(); +} + +QString CrashReporter::describeDsn(const QString &dsn) +{ + const QRegularExpressionMatch match = dsnPattern().match(dsn); + if (!match.hasMatch()) { + return QString(); + } + // Host and project id only: the key in group 2 must never reach a log line. + return QStringLiteral("%1 (project %2)").arg(match.captured(3), match.captured(4)); +} + +QString CrashReporter::describeOriginMismatch(const QString &envelopePath, const QString ¤tDsn) +{ + const QString destination = describeDsn(currentDsn); + if (destination.isEmpty()) { + return QString(); + } + + QFile file(envelopePath); + if (!file.open(QIODevice::ReadOnly)) { + return QString(); + } + // The envelope header is the first line and is small; the cap only keeps a corrupt file from + // being read in full. + const QByteArray headerLine = file.readLine(4096).trimmed(); + if (headerLine.isEmpty() || !headerLine.startsWith('{')) { + return QString(); + } + const QString originDsn = QJsonDocument::fromJson(headerLine).object().value(QStringLiteral("dsn")).toString(); + const QString origin = describeDsn(originDsn); + // Equal destinations are not a mismatch: a different key for the same host and project still + // uploads where the crash was captured. + if (origin.isEmpty() || origin == destination) { + return QString(); + } + return QStringLiteral("was captured for %1 but this session uploads with %2").arg(origin, destination); +} + bool CrashReporter::isReportingEnabled(const QString &dsn, PersonalSettings::CrashReportingMode consentMode) { return !dsn.isEmpty() && consentMode != PersonalSettings::CrashReportingMode::NeverSend; @@ -164,10 +227,28 @@ void CrashReporter::init(const QString &dsn, shutdown(); sentryDatabasePath = databasePath; if (!isReportingEnabled(dsn, consentMode)) { + if (!dsn.isEmpty()) { + // A deliberate choice rather than a misconfiguration, so it is not a warning. The + // no-DSN case is reported by the caller, which is the only place that knows whether + // the environment or the build supplied the DSN. + qCInfo(CrashReportLog) << "crash reporting disabled by preference (Never send)"; + } return; } // Only created when the SDK will actually run: no-DSN launches stay side-effect free. #if COCKATRICE_WITH_CRASH_REPORTING + // A crash captured by an earlier run is uploaded by THIS session's transport, which posts every + // envelope with the DSN it was armed with (sentry-native keeps the DSN in the transport state, + // and envelopes restored from disk are raw, so their own "dsn" header is never read). When the + // two disagree the report lands in a project nobody chose, which no other log line reveals. + const QStringList pendingEnvelopes = pendingEnvelopePaths(sentryDatabasePath); + for (const QString &envelopePath : pendingEnvelopes) { + const QString mismatch = describeOriginMismatch(envelopePath, dsn); + if (!mismatch.isEmpty()) { + qCWarning(CrashReportLog) << "pending crash envelope" << envelopePath << mismatch; + } + } + qCInfo(CrashReportLog) << "crash reporting armed for" << describeDsn(dsn); QDir().mkpath(sentryDatabasePath); if (consentMode != PersonalSettings::CrashReportingMode::AlwaysSend) { // sentry-native persists consent to /user-consent and reloads it in sentry_init @@ -188,6 +269,9 @@ void CrashReporter::init(const QString &dsn, ready = (sentry_init(options) == 0); #else Q_UNUSED(release); + // Not a misconfiguration like an empty DSN: this build cannot report whatever the DSN says, so + // a tester forcing a crash needs to know the difference. + qCInfo(CrashReportLog) << "crash reporting unavailable: built without Sentry support"; ready = false; #endif } diff --git a/cockatrice/src/client/crash/crash_reporter.h b/cockatrice/src/client/crash/crash_reporter.h index 6ddef6d1e..598b6445c 100644 --- a/cockatrice/src/client/crash/crash_reporter.h +++ b/cockatrice/src/client/crash/crash_reporter.h @@ -110,6 +110,36 @@ public: */ static bool isReportingEnabled(const QString &dsn, PersonalSettings::CrashReportingMode consentMode); + /** + * @brief Whether a DSN has the shape the build and the SDK require: + * ://@/. + * + * Deliberately the same unanchored pattern cmake/CrashReporting.cmake matches (CMake's MATCHES + * searches rather than anchoring), so a DSN this build accepted is never rejected here. The SDK + * refuses to even build a request for a DSN it considers invalid, so an unchecked typo disables + * uploading with no error anywhere. + */ + static bool isDsnWellFormed(const QString &dsn); + + /** + * @brief Human-readable destination of a DSN, e.g. "o0.ingest.sentry.io (project 1234567)". + * + * Safe to log: the public key is never part of the result. Empty for a malformed DSN. + */ + static QString describeDsn(const QString &dsn); + + /** + * @brief Describe the captured-for DSN of a pending envelope when it points at a different + * destination than the DSN this session would upload it with; empty when they agree. + * + * sentry-native posts every envelope with the DSN held in the transport state of the session + * doing the sending (envelopes restored from disk are raw and their own "dsn" header is never + * read), so a cached crash envelope captured for one project can silently be uploaded to + * another - the exact way a tester's report lands in the wrong place. Compares destinations + * (host and project id), not raw strings, so a different key for the same project stays quiet. + */ + static QString describeOriginMismatch(const QString &envelopePath, const QString ¤tDsn); + static QString sanitizeLogLine(const QString &line); static QString redactCockatriceUrls(const QString &line); diff --git a/cockatrice/src/main.cpp b/cockatrice/src/main.cpp index 64994eb99..fd98b1945 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -285,9 +285,29 @@ int main(int argc, char *argv[]) int crashLogAttachedEnvelopes = 0; { QString crashDsn = QString::fromUtf8(qgetenv("COCKATRICE_SENTRY_DSN")); - if (crashDsn.isEmpty()) { + const bool dsnFromEnvironment = !crashDsn.isEmpty(); + if (!dsnFromEnvironment) { crashDsn = QString::fromUtf8(COCKATRICE_SENTRY_DSN); } + if (crashDsn.isEmpty()) { + // Nothing is armed without a DSN, so no crash is ever captured and nothing is ever sent. + // It is the one disabled configuration a tester cannot see from the UI, so say it here. + qCWarning(CrashReportLog) << "crash reporting disabled: no DSN configured (COCKATRICE_SENTRY_DSN is " + "not set and this build has no DSN baked in)"; + } else if (!CrashReporter::isDsnWellFormed(crashDsn)) { + // Same rule the installer applies (cmake/CrashReporting.cmake): a malformed DSN disables + // reporting rather than silently falling back to the project baked into the build, which + // would upload where the tester never chose. The SDK would otherwise refuse to build a + // request at all and report nothing about it. + qCWarning(CrashReportLog) << "crash reporting disabled:" + << (dsnFromEnvironment ? "COCKATRICE_SENTRY_DSN is set but malformed" + : "the DSN baked into this build is malformed") + << "(expected ://@/)"; + crashDsn.clear(); + } else if (dsnFromEnvironment) { + // Destination and mode are logged by CrashReporter::init() once it resolves them. + qCInfo(CrashReportLog) << "using COCKATRICE_SENTRY_DSN from the environment"; + } const PersonalSettings::CrashReportingMode crashMode = SettingsCache::instance().personal().getCrashReportingMode(); // AlwaysSend folds the crashed session's redacted log into the pending crash diff --git a/tests/crash_reporter_test.cpp b/tests/crash_reporter_test.cpp index 2f311ded3..e15007f7a 100644 --- a/tests/crash_reporter_test.cpp +++ b/tests/crash_reporter_test.cpp @@ -75,6 +75,68 @@ TEST(CrashReporterTest, ReportingEnabledGating) ASSERT_FALSE(CrashReporter::isReportingEnabled(dsn, Mode::NeverSend)); } +TEST(CrashReporterTest, IsDsnWellFormedAcceptsWhatTheBuildAcceptsAndRejectsUiLinks) +{ + ASSERT_TRUE(CrashReporter::isDsnWellFormed(QStringLiteral("https://abc123@o0.ingest.sentry.io/1234567"))); + ASSERT_TRUE(CrashReporter::isDsnWellFormed(QStringLiteral("http://key@localhost:9000/42"))); + // Unanchored like cmake/CrashReporting.cmake's MATCHES, so a DSN the build baked is never + // rejected here: a trailing path still matches. + ASSERT_TRUE(CrashReporter::isDsnWellFormed(QStringLiteral("https://key@host.example/7/extra"))); + // The project-settings URL is the mistake a tester actually makes. + ASSERT_FALSE(CrashReporter::isDsnWellFormed( + QStringLiteral("https://sentry.io/settings/cockatrice/projects/cockatrice-desktop/keys/"))); + ASSERT_FALSE(CrashReporter::isDsnWellFormed(QStringLiteral("https://key@host.example/not-a-number"))); + ASSERT_FALSE(CrashReporter::isDsnWellFormed(QStringLiteral("o0.ingest.sentry.io/1234567"))); + ASSERT_FALSE(CrashReporter::isDsnWellFormed(QString())); +} + +TEST(CrashReporterTest, DescribeDsnNamesHostAndProjectWithoutTheKey) +{ + const QString described = + CrashReporter::describeDsn(QStringLiteral("https://supersecretkey@o0.ingest.sentry.io/1234567")); + ASSERT_EQ(described, QStringLiteral("o0.ingest.sentry.io (project 1234567)")); + ASSERT_FALSE(described.contains(QStringLiteral("supersecretkey"))); + ASSERT_TRUE(CrashReporter::describeDsn(QStringLiteral("not a dsn")).isEmpty()); + ASSERT_TRUE(CrashReporter::describeDsn(QString()).isEmpty()); +} + +TEST(CrashReporterTest, DescribeOriginMismatchReportsOnlyRealDestinationChanges) +{ + const QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString envelopePath = dir.filePath(QStringLiteral("crash.envelope")); + QFile file(envelopePath); + ASSERT_TRUE(file.open(QIODevice::WriteOnly)); + file.write(QByteArrayLiteral("{\"event_id\":\"x\",\"dsn\":\"https://keyA@hostA.example/111\"}\n")); + file.write(QByteArrayLiteral("{\"type\":\"event\",\"length\":2}\n{}\n")); + file.close(); + + const QString described = + CrashReporter::describeOriginMismatch(envelopePath, QStringLiteral("https://keyB@hostB.example/222")); + ASSERT_FALSE(described.isEmpty()); + ASSERT_TRUE(described.contains(QStringLiteral("hostA.example"))); + ASSERT_TRUE(described.contains(QStringLiteral("hostB.example"))); + ASSERT_FALSE(described.contains(QStringLiteral("keyA"))); + ASSERT_FALSE(described.contains(QStringLiteral("keyB"))); + + // A different key for the SAME host and project is not a routing change and must stay quiet. + ASSERT_TRUE( + CrashReporter::describeOriginMismatch(envelopePath, QStringLiteral("https://otherkey@hostA.example/111")) + .isEmpty()); + // An unreadable envelope, a header without a dsn, or no current dsn are all silent. + ASSERT_TRUE(CrashReporter::describeOriginMismatch(dir.filePath(QStringLiteral("absent.envelope")), + QStringLiteral("https://keyB@hostB.example/222")) + .isEmpty()); + ASSERT_TRUE(CrashReporter::describeOriginMismatch(envelopePath, QString()).isEmpty()); + const QString headerlessPath = dir.filePath(QStringLiteral("no-dsn.envelope")); + QFile headerless(headerlessPath); + ASSERT_TRUE(headerless.open(QIODevice::WriteOnly)); + headerless.write(QByteArrayLiteral("{\"event_id\":\"y\"}\n")); + headerless.close(); + ASSERT_TRUE(CrashReporter::describeOriginMismatch(headerlessPath, QStringLiteral("https://keyB@hostB.example/222")) + .isEmpty()); +} + TEST(CrashReporterTest, ShutdownWithoutInitIsSafe) { CrashReporter reporter;