jeirmeister/Cockatrice
1
0
Fork 0
mirror of https://github.com/Cockatrice/Cockatrice.git synced 2026-11-08 02:53:25 -08:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

[CrashReporting] Log where crash reports go and warn when they cannot

Browse source 
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.
This commit is contained in:
Lukas Brübach 2026-10-18 08:26:05 +02:00 • committed by GitHub
parent 828e6cc1d0
commit 69b421f1c2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 197 additions and 1 deletions
Download patch file Download diff file Expand all files Collapse all files

84
cockatrice/src/client/crash/crash_reporter.cpp
View file

@ -7,6 +7,8 @@
#include <QByteArray>
#include <QDir>
#include <QFile>
#include <QJsonDocument>
#include <QJsonObject>
#include <QLoggingCategory>
#include <QRegularExpression>
#include <QUrl>
@ -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 &currentDsn)
{
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 <database>/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
}

30
cockatrice/src/client/crash/crash_reporter.h
View file

@ -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:
* <scheme>://<key>@<host>/<project-id>.
*
* 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 &currentDsn);
static QString sanitizeLogLine(const QString &line);
static QString redactCockatriceUrls(const QString &line);

22
cockatrice/src/main.cpp
View file

@ -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 <scheme>://<key>@<host>/<project-id>)";
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

62
tests/crash_reporter_test.cpp
View file

@ -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;