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;