From 6385f3fab38c910917b5d03f23e25f69f285bbfd Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 19 Oct 2026 14:57:03 +0200 Subject: [PATCH 01/90] [CrashReporting] Mock crash-report dialog and reporting preference (#7521) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [CrashReporting] Mock dialog, sentry skeleton and CI prerequisites Mock: DlgCrashReport preview dialog (no account needed, redacted log preview), CrashReportingMode Ask/Always/Never setting in DebugSettings, --simulate-crash flag and Help-menu preview entry. Send writes a local mock JSON only. Vendor sentry-native 0.17.3 via FetchContent behind WITH_CRASH_REPORTING (breakpad everywhere, backend none under sanitizers) plus a CrashReporter facade with early init and post-loop shutdown. DSN is a placeholder: empty CMake default with COCKATRICE_SENTRY_DSN env override, so SDK init is skipped and mock behavior is unchanged until a real DSN is configured. CI prerequisites riding here so every layer below builds green: libcurl dev plus CA bundles in the Linux images and CodeQL env, sentry Qt integration off (its own find_package cannot see the macOS thin Qt layout), and the StrFunc Using construction makensis requires. Tests: ctest green, format clean, code plus UI gates PASS. * [CrashReporting] Forward mode changes to SDK and fix Linux CI opt-in Connects PersonalSettings::crashReportingModeChanged to giveConsent()/revokeConsent() so switching from AlwaysSend to NeverSend (or Ask) mid-session reaches the SDK instead of leaving consent given until restart. Forwards WITH_CRASH_REPORTING into the docker container the way CCACHE_DIR is, so the Linux package leg actually builds with crash reporting ON instead of silently building OFF. * [CrashReporting] Give crash-report logging its own category per review * [CrashReporting] Drop unused DebugSettings constructor visibility per review * [CrashReporting] Document reporting modes on the enum per review * [CrashReporting] Match the dump scan to the writer's temp fallback Review nit: when the handler's SHGetKnownFolderPath lookup fails it writes to /../.., but the startup scan looked in QDir::tempPath(), so a dump produced through the fallback would never be found and its crash would never offer to be reported. Walk up the same two levels in the scan. The path is Windows-only (#ifdef Q_OS_WIN); the statements were compiled and run standalone against Qt 6 to confirm the API use. * [CrashReporting] Complete the dump-scan fallback path The previous commit walked the scan's fallback up two levels but dropped the CrashDumps/Cockatrice suffix that the writer appends unconditionally, so a dump written through the fallback was still invisible to the scan. Evaluate the writer's expression verbatim rather than mirroring it with QDir: cdUp() is existence-checked and silently leaves the path alone when the directory is missing, parent_path() is not, and QDir::absoluteFilePath() joins the shared suffix without risking a doubled separator when the walk lands on a root. Verified by compiling the block verbatim against Qt 6 with -Wextra -Werror and comparing it with the writer's join for both the normal and the fallback case. Also record that %LOCALAPPDATA% is the scan's stand-in for FOLDERID_LocalAppData and why the two normally agree. --------- Co-authored-by: Lukas Brübach --- .ci/Debian12/Dockerfile | 2 + .ci/Debian13/Dockerfile | 1 + .ci/Fedora43/Dockerfile | 2 + .ci/Fedora44/Dockerfile | 2 + .ci/Ubuntu24.04/Dockerfile | 2 + .ci/Ubuntu26.04/Dockerfile | 1 + .ci/compile.sh | 6 + .ci/docker.sh | 6 + .github/workflows/codeql.yml | 2 + .github/workflows/desktop-build.yml | 3 + CMakeLists.txt | 1 + cmake/CrashReporting.cmake | 124 ++++++++++++++ cockatrice/CMakeLists.txt | 4 + .../src/client/crash/crash_reporter.cpp | 119 ++++++++++++++ cockatrice/src/client/crash/crash_reporter.h | 66 ++++++++ cockatrice/src/interface/logger.cpp | 144 ++++++++++++++++- cockatrice/src/interface/logger.h | 12 +- .../widgets/dialogs/dlg_crash_report.cpp | 152 ++++++++++++++++++ .../widgets/dialogs/dlg_crash_report.h | 51 ++++++ .../settings_page/general_settings_page.cpp | 30 +++- .../settings_page/general_settings_page.h | 4 + cockatrice/src/main.cpp | 96 ++++++++++- .../libcockatrice/settings/debug_settings.cpp | 2 +- .../settings/personal_settings.cpp | 21 +++ .../settings/personal_settings.h | 11 ++ tests/CMakeLists.txt | 15 ++ tests/crash_reporter_test.cpp | 87 ++++++++++ tests/settings/settings_defaults_test.cpp | 27 ++++ 28 files changed, 988 insertions(+), 5 deletions(-) create mode 100644 cmake/CrashReporting.cmake create mode 100644 cockatrice/src/client/crash/crash_reporter.cpp create mode 100644 cockatrice/src/client/crash/crash_reporter.h create mode 100644 cockatrice/src/interface/widgets/dialogs/dlg_crash_report.cpp create mode 100644 cockatrice/src/interface/widgets/dialogs/dlg_crash_report.h create mode 100644 tests/crash_reporter_test.cpp diff --git a/.ci/Debian12/Dockerfile b/.ci/Debian12/Dockerfile index 14c6f018c..4da910dc0 100644 --- a/.ci/Debian12/Dockerfile +++ b/.ci/Debian12/Dockerfile @@ -3,11 +3,13 @@ FROM debian:12 RUN apt-get update && \ DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ build-essential \ + ca-certificates \ ccache \ cmake \ file \ g++ \ git \ + libcurl4-openssl-dev \ libgl-dev \ liblzma-dev \ libmariadb-dev-compat \ diff --git a/.ci/Debian13/Dockerfile b/.ci/Debian13/Dockerfile index b597307c5..94f711398 100644 --- a/.ci/Debian13/Dockerfile +++ b/.ci/Debian13/Dockerfile @@ -9,6 +9,7 @@ RUN apt-get update && \ file \ g++ \ git \ + libcurl4-openssl-dev \ libgl-dev \ liblzma-dev \ libmariadb-dev-compat \ diff --git a/.ci/Fedora43/Dockerfile b/.ci/Fedora43/Dockerfile index 463da5a51..8aa345651 100644 --- a/.ci/Fedora43/Dockerfile +++ b/.ci/Fedora43/Dockerfile @@ -1,10 +1,12 @@ FROM fedora:43 RUN dnf install -y \ + ca-certificates \ ccache \ cmake \ gcc-c++ \ git \ + libcurl-devel \ mariadb-devel \ ninja-build \ openssl-devel \ diff --git a/.ci/Fedora44/Dockerfile b/.ci/Fedora44/Dockerfile index 62238e760..15204fa1a 100644 --- a/.ci/Fedora44/Dockerfile +++ b/.ci/Fedora44/Dockerfile @@ -1,10 +1,12 @@ FROM fedora:44 RUN dnf install -y \ + ca-certificates \ ccache \ cmake \ gcc-c++ \ git \ + libcurl-devel \ mariadb-devel \ ninja-build \ openssl-devel \ diff --git a/.ci/Ubuntu24.04/Dockerfile b/.ci/Ubuntu24.04/Dockerfile index b9fb2b701..43497dd3b 100644 --- a/.ci/Ubuntu24.04/Dockerfile +++ b/.ci/Ubuntu24.04/Dockerfile @@ -3,11 +3,13 @@ FROM ubuntu:24.04 RUN apt-get update && \ DEBIAN_FRONTEND=noninteractive apt-get install -y --no-install-recommends \ build-essential \ + ca-certificates \ ccache \ cmake \ file \ g++ \ git \ + libcurl4-openssl-dev \ libgl-dev \ liblzma-dev \ libmariadb-dev-compat \ diff --git a/.ci/Ubuntu26.04/Dockerfile b/.ci/Ubuntu26.04/Dockerfile index cc6528858..4dc861ae9 100644 --- a/.ci/Ubuntu26.04/Dockerfile +++ b/.ci/Ubuntu26.04/Dockerfile @@ -10,6 +10,7 @@ RUN apt-get update && \ g++ \ gcovr \ git \ + libcurl4-openssl-dev \ libgl-dev \ liblzma-dev \ libmariadb-dev-compat \ diff --git a/.ci/compile.sh b/.ci/compile.sh index 6789b9817..2aeeaa6ec 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -200,6 +200,12 @@ if [[ $USE_VCPKG ]]; then flags+=("-DUSE_VCPKG=1") flags+=("-DVCPKG_INSTALL_OPTIONS=--x-abi-tools-use-exact-versions") fi +if [[ ${WITH_CRASH_REPORTING:-} == "ON" ]]; then + # Release CI opts into the sentry-native SDK (default is OFF so from-source, distro and + # offline package builds never fetch it). Debug builds stay SDK-free so crashes from + # developer builds never reach Sentry. + flags+=("-DWITH_CRASH_REPORTING=ON") +fi # Add cmake --build flags buildflags=(--config "$BUILDTYPE") diff --git a/.ci/docker.sh b/.ci/docker.sh index 412ad6a1d..64ea6baa7 100644 --- a/.ci/docker.sh +++ b/.ci/docker.sh @@ -175,6 +175,12 @@ function RUN () args+=(--mount "type=bind,source=$GITHUB_OUTPUT,target=/gh_output") args+=(--env "GITHUB_OUTPUT=/gh_output") fi + # The container does not inherit host env vars: without this, WITH_CRASH_REPORTING=ON set in + # the workflow never reaches compile.sh inside the container, so the Linux packages silently + # build with crash reporting OFF while the libcurl packages in the Dockerfiles go unused. + if [[ ${WITH_CRASH_REPORTING:-} ]]; then + args+=(--env "WITH_CRASH_REPORTING=${WITH_CRASH_REPORTING}") + fi # shellcheck disable=2086 docker run "${args[@]}" $RUN_ARGS "$IMAGE_NAME" bash "$BUILD_SCRIPT" $RUN_OPTS "$@" return $? diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 75fbc59f1..75494dc3e 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -60,8 +60,10 @@ jobs: run: | sudo apt-get update sudo apt-get install -y --no-install-recommends \ + ca-certificates \ cmake \ g++ \ + libcurl4-openssl-dev \ libgl-dev \ liblzma-dev \ libmariadb-dev-compat \ diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 0b9502fe3..2215ac2d2 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -239,6 +239,7 @@ jobs: SUFFIX: '-${{ matrix.distro }}${{ matrix.version }}' package: '${{ matrix.package }}' server_only: '${{ matrix.server_only }}' + WITH_CRASH_REPORTING: 'ON' run: | source .ci/docker.sh args=() @@ -558,6 +559,8 @@ jobs: VCPKG_DISABLE_METRICS: 1 VCPKG_FEATURE_FLAGS: dependencygraph VCPKG_TARGET_MACOS_VERSION: ${{ matrix.vcpkg_deployment_target }} + # Release legs compile the sentry-native SDK; Debug legs stay SDK-free. + WITH_CRASH_REPORTING: ${{ matrix.type == 'Release' && 'ON' || '' }} run: .ci/compile.sh --server --test --vcpkg # CPack reports a failed makensis run as "Problem running NSIS command" diff --git a/CMakeLists.txt b/CMakeLists.txt index 87f1c746a..b294f27ba 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -220,6 +220,7 @@ endif() # when ENABLE_SANITIZERS or ENABLE_COVERAGE is on. include(Sanitizers) include(Coverage) +include(CrashReporting) # GNU systems need to define the Mersenne exponent for the RNG to compile w/o warning if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") diff --git a/cmake/CrashReporting.cmake b/cmake/CrashReporting.cmake new file mode 100644 index 000000000..a728e25c2 --- /dev/null +++ b/cmake/CrashReporting.cmake @@ -0,0 +1,124 @@ +# Crash reporting via sentry-native (Sentry protocol, MIT). +# Included once from the root CMakeLists.txt, right after the sanitizer/coverage helpers. +# +# Inputs: +# WITH_CRASH_REPORTING -- build the client with the sentry-native SDK (default OFF, our release +# CI passes ON; packagers and from-source builds stay SDK-free until this +# ships something real, so offline package builds never fetch the SDK) +# COCKATRICE_SENTRY_DSN -- Sentry DSN baked into the build (default empty = SDK init skipped at runtime) +# COCKATRICE_SENTRY_TAG -- sentry-native release tag to vendor via FetchContent +# ENABLE_SANITIZERS -- forces the sentry backend to "none" (crashpad vs ASan conflict) +# Defines: cockatrice_target_enable_crash_reporting() + +option(WITH_CRASH_REPORTING "Build with crash reporting via sentry-native" OFF) +set(COCKATRICE_SENTRY_DSN + "" + CACHE STRING "Sentry DSN baked into the client; empty disables crash-report upload at runtime" +) +# Pinned stable release; tag verified at https://github.com/getsentry/sentry-native/tags. +set(COCKATRICE_SENTRY_TAG + "0.17.3" + CACHE STRING "sentry-native release tag to vendor via FetchContent" +) + +# Wire the SDK (or the no-op fallback) into a client target. Always callable: with the +# option OFF the compile definitions below make crash_reporter.cpp compile to no-ops. +function(cockatrice_target_enable_crash_reporting _target) + if(WITH_CRASH_REPORTING AND TARGET sentry::sentry) + target_link_libraries(${_target} PRIVATE sentry::sentry) + target_compile_definitions(${_target} PRIVATE COCKATRICE_WITH_CRASH_REPORTING=1) + else() + target_compile_definitions(${_target} PRIVATE COCKATRICE_WITH_CRASH_REPORTING=0) + endif() + # Always baked in (possibly empty): main() prefers the COCKATRICE_SENTRY_DSN env var at runtime. + target_compile_definitions(${_target} PRIVATE "COCKATRICE_SENTRY_DSN=\"${COCKATRICE_SENTRY_DSN}\"") +endfunction() + +if(WITH_CRASH_REPORTING AND WITH_CLIENT) + include(FetchContent) + + if(ENABLE_SANITIZERS) + # The crash handler cannot run under ASan/UBSan, so there is no backend to capture with. + set(_sentry_backend "none") + elseif(WIN32 OR APPLE) + # Out-of-process handler; shipping the handler binary is a later increment, which unifies + # every OS on breakpad (in-process, no extra binary to ship). + set(_sentry_backend "crashpad") + else() + # In-process, no extra binary to ship. + set(_sentry_backend "breakpad") + endif() + # FORCE so toggling ENABLE_SANITIZERS on a reused build dir cannot leave a stale backend behind. + set(SENTRY_BACKEND + "${_sentry_backend}" + CACHE STRING "Crash handler backend for sentry-native" FORCE + ) + unset(_sentry_backend) + + # No Qt integration: it runs find_package(Qt6) inside the vendored build, which + # cannot see our thin/aqt Qt layouts (macOS CI) under foreign toolchains, and its + # breadcrumbs could carry unredacted log lines into native crash events that bypass + # before_send. Our attached redacted log preview already carries the log context. + set(SENTRY_INTEGRATION_QT + OFF + CACHE BOOL "Qt log-message breadcrumbs for sentry-native (off: discovery + secret surface)" FORCE + ) + # The transport stays at the SDK default (curl on Apple/Linux, winhttp on Windows). + set(SENTRY_BUILD_TESTS + OFF + CACHE BOOL "Build sentry-native tests" FORCE + ) + set(SENTRY_BUILD_EXAMPLES + OFF + CACHE BOOL "Build sentry-native examples" FORCE + ) + + fetchcontent_declare( + sentry + GIT_REPOSITORY https://github.com/getsentry/sentry-native.git + GIT_TAG ${COCKATRICE_SENTRY_TAG} + ) + fetchcontent_makeavailable(sentry) + + # Vendored sentry-native (and its breakpad/crashpad trees) warn under our -Wall -Wextra + # (e.g. breakpad's -Wmissing-field-initializers), and WARNING_AS_ERROR would fail the build + # on code we cannot fix. Those findings are not actionable for us, so opt the whole vendored + # tree out of warnings-as-errors while keeping the warnings themselves visible; our own + # targets keep -Werror. Interface libraries compile nothing, so they are skipped outright: + # an INTERFACE -Wno-error would propagate to our consumers and weaken their build. + function(_cockatrice_sentry_opt_out_of_werror _dir) + get_property( + _sentry_targets + DIRECTORY "${_dir}" + PROPERTY BUILDSYSTEM_TARGETS + ) + foreach(_sentry_target IN LISTS _sentry_targets) + get_target_property(_sentry_type ${_sentry_target} TYPE) + if(_sentry_type STREQUAL "INTERFACE_LIBRARY") + continue() + elseif(_sentry_type MATCHES "LIBRARY|EXECUTABLE") + get_target_property(_sentry_imported ${_sentry_target} IMPORTED) + if(NOT _sentry_imported) + target_compile_options(${_sentry_target} PRIVATE -Wno-error) + endif() + endif() + endforeach() + get_property( + _sentry_subdirs + DIRECTORY "${_dir}" + PROPERTY SUBDIRECTORIES + ) + foreach(_sentry_subdir IN LISTS _sentry_subdirs) + _cockatrice_sentry_opt_out_of_werror("${_sentry_subdir}") + endforeach() + endfunction() + if(NOT MSVC) + _cockatrice_sentry_opt_out_of_werror("${sentry_SOURCE_DIR}") + endif() + + message(STATUS "Crash reporting: sentry-native ${COCKATRICE_SENTRY_TAG} (backend: ${SENTRY_BACKEND})") +else() + message(STATUS "Crash reporting: SDK not vendored " + "(WITH_CRASH_REPORTING=${WITH_CRASH_REPORTING}, WITH_CLIENT=${WITH_CLIENT})" + ) +endif() diff --git a/cockatrice/CMakeLists.txt b/cockatrice/CMakeLists.txt index 5baf10b1e..bec858dcb 100644 --- a/cockatrice/CMakeLists.txt +++ b/cockatrice/CMakeLists.txt @@ -15,6 +15,7 @@ set(cockatrice_SOURCES src/client/network/update/client/client_update_checker.cpp src/client/network/update/client/release_channel.cpp src/client/network/update/card_spoiler/spoiler_background_updater.cpp + src/client/crash/crash_reporter.cpp src/client/latency_graph_widget.cpp src/client/latency_status_widget.cpp src/client/lag_monitor.cpp @@ -30,6 +31,7 @@ set(cockatrice_SOURCES src/interface/widgets/dialogs/dlg_connect.cpp src/interface/widgets/dialogs/dlg_convert_deck_to_cod_format.cpp src/interface/widgets/dialogs/dlg_create_game.cpp + src/interface/widgets/dialogs/dlg_crash_report.cpp src/interface/widgets/dialogs/dlg_default_tags_editor.cpp src/interface/widgets/dialogs/dlg_edit_avatar.cpp src/interface/widgets/dialogs/dlg_edit_password.cpp @@ -578,6 +580,8 @@ target_link_libraries( ${COCKATRICE_QT_MODULES} ) +cockatrice_target_enable_crash_reporting(cockatrice) + if(UNIX) if(APPLE) set(MACOSX_BUNDLE_INFO_STRING "${PROJECT_NAME}") diff --git a/cockatrice/src/client/crash/crash_reporter.cpp b/cockatrice/src/client/crash/crash_reporter.cpp new file mode 100644 index 000000000..fe1678b11 --- /dev/null +++ b/cockatrice/src/client/crash/crash_reporter.cpp @@ -0,0 +1,119 @@ +#include "crash_reporter.h" + +#if COCKATRICE_WITH_CRASH_REPORTING +#include +#endif + +#include +#include +#include +#include +#include +#include + +Q_LOGGING_CATEGORY(CrashReportLog, "crash_report"); + +bool CrashReporter::isReportingEnabled(const QString &dsn, PersonalSettings::CrashReportingMode consentMode) +{ + return !dsn.isEmpty() && consentMode != PersonalSettings::CrashReportingMode::NeverSend; +} + +void CrashReporter::init(const QString &dsn, + const QString &release, + const QString &databasePath, + PersonalSettings::CrashReportingMode consentMode) +{ + shutdown(); + sentryDatabasePath = databasePath; + if (!isReportingEnabled(dsn, consentMode)) { + return; + } + // Only created when the SDK will actually run: no-DSN launches stay side-effect free. +#if COCKATRICE_WITH_CRASH_REPORTING + QDir().mkpath(sentryDatabasePath); + if (consentMode != PersonalSettings::CrashReportingMode::AlwaysSend) { + // sentry-native persists consent to /user-consent and reloads it in sentry_init + // before old runs are processed, so a stale "given" from an earlier AlwaysSend run (or an + // earlier Send in Ask mode) would upload the next crash before the dialog asks. Our + // CrashReportingMode is the source of truth, so clear the persisted state first; asking + // afterwards re-establishes it via giveConsent()/revokeConsent(). + QFile::remove(sentryDatabasePath + QStringLiteral("/user-consent")); + } + sentry_options_t *options = sentry_options_new(); + sentry_options_set_dsn(options, dsn.toUtf8().constData()); + sentry_options_set_release(options, release.toUtf8().constData()); + sentry_options_set_database_path(options, sentryDatabasePath.toUtf8().constData()); + // Consent-gated uploads: crashes are captured into the local database but + // nothing leaves the machine until giveConsent(). + sentry_options_set_require_user_consent(options, 1); + ready = (sentry_init(options) == 0); +#else + Q_UNUSED(release); + ready = false; +#endif +} + +bool CrashReporter::isReady() const +{ + return ready; +} + +void CrashReporter::shutdown() +{ +#if COCKATRICE_WITH_CRASH_REPORTING + if (ready) { + sentry_close(); + } +#endif + ready = false; +} + +void CrashReporter::giveConsent() +{ +#if COCKATRICE_WITH_CRASH_REPORTING + if (ready) { + sentry_user_consent_give(); + } +#endif +} + +void CrashReporter::revokeConsent() +{ +#if COCKATRICE_WITH_CRASH_REPORTING + if (ready) { + sentry_user_consent_revoke(); + } +#endif +} + +QString CrashReporter::sanitizeLogLine(const QString &line) +{ + static const auto colorEscapeCodePattern = QRegularExpression(QStringLiteral("\033\\[[0-9;]*m")); + + QString sanitized = line; + sanitized.replace(colorEscapeCodePattern, ""); + return sanitized; +} + +QString CrashReporter::redactCockatriceUrls(const QString &line) +{ + static auto urlPattern = QRegularExpression(QStringLiteral("cockatrice://\\S+")); + + // Single pass over the original line: rescanning the replaced text would match a redacted + // secret-free URL again and loop forever. + QString redacted; + redacted.reserve(line.size()); + qsizetype pos = 0; + QRegularExpressionMatchIterator it = urlPattern.globalMatch(line); + while (it.hasNext()) { + QRegularExpressionMatch match = it.next(); + redacted.append(line.mid(pos, match.capturedStart() - pos)); + // The pattern only matches cockatrice:// URLs, so the check is folded into the parse: + // scheme://host, secrets dropped. + const QUrl parsed(match.captured(0)); + redacted.append(parsed.scheme() + QStringLiteral("://") + parsed.host()); + pos = match.capturedEnd(); + } + redacted.append(line.mid(pos)); + return redacted; +} diff --git a/cockatrice/src/client/crash/crash_reporter.h b/cockatrice/src/client/crash/crash_reporter.h new file mode 100644 index 000000000..f57ec6019 --- /dev/null +++ b/cockatrice/src/client/crash/crash_reporter.h @@ -0,0 +1,66 @@ +/** + * @file crash_reporter.h + * @ingroup Core + */ + +#ifndef CRASH_REPORTER_H +#define CRASH_REPORTER_H + +#include +#include +#include + +#ifndef COCKATRICE_WITH_CRASH_REPORTING +#define COCKATRICE_WITH_CRASH_REPORTING 0 +#endif + +#ifndef COCKATRICE_SENTRY_DSN +#define COCKATRICE_SENTRY_DSN "" +#endif + +Q_DECLARE_LOGGING_CATEGORY(CrashReportLog); + +/** + * @brief Sentry-backed crash-report facade, owned by main(). + * + * Single responsibility: init/shutdown the sentry-native SDK behind consent + * gating, plus the shared log redaction helpers. An empty DSN (or Never + * consent) skips the SDK entirely, so the mock dialog fallback behaves exactly + * as before. init() arms require-user-consent so cached crash envelopes never + * upload before the user is asked; giveConsent()/revokeConsent() (driven by + * the reporting-mode setting and the crash dialog) gate uploads, and any stale + * persisted consent from a previous AlwaysSend run is cleared before init when + * the mode is not AlwaysSend. The real send path is a later increment. All SDK + * calls are guarded so OFF builds compile to no-ops. + */ +class CrashReporter +{ +public: + CrashReporter() = default; + CrashReporter(const CrashReporter &) = delete; + CrashReporter &operator=(const CrashReporter &) = delete; + ~CrashReporter() = default; + + void init(const QString &dsn, + const QString &release, + const QString &databasePath, + PersonalSettings::CrashReportingMode consentMode); + [[nodiscard]] bool isReady() const; + void shutdown(); + void giveConsent(); + void revokeConsent(); + + /** + * @brief SDK-independent gating decision, unit-testable in OFF builds. + */ + static bool isReportingEnabled(const QString &dsn, PersonalSettings::CrashReportingMode consentMode); + + static QString sanitizeLogLine(const QString &line); + static QString redactCockatriceUrls(const QString &line); + +private: + bool ready = false; + QString sentryDatabasePath; +}; + +#endif diff --git a/cockatrice/src/interface/logger.cpp b/cockatrice/src/interface/logger.cpp index 87fdcbd8c..864840f4e 100644 --- a/cockatrice/src/interface/logger.cpp +++ b/cockatrice/src/interface/logger.cpp @@ -5,6 +5,8 @@ #include #include #include +#include +#include #include #include #include @@ -18,8 +20,11 @@ #define LOGGER_MAX_ENTRIES 128 #define LOGGER_FILENAME "qdebug.txt" +#define LOGGER_SESSION_MARKER "Log session started at" +#define LOGGER_PERSISTENT_MAX_BYTES (512 * 1024) +#define LOGGER_PERSISTENT_KEEP_LINES 2000 -Logger::Logger() : logToFileEnabled(false) +Logger::Logger() : logToFileEnabled(false), persistentLogEnabled(false) { logBuffer.append(getClientVersion()); logBuffer.append(getSystemArchitecture()); @@ -35,6 +40,7 @@ Logger::Logger() : logToFileEnabled(false) Logger::~Logger() { closeLogfileSession(); + closePersistentLogSession(); logBuffer.clear(); } @@ -82,6 +88,137 @@ void Logger::closeLogfileSession() fileHandle.close(); } +void Logger::setPersistentLogPath(const QString &path) +{ + QMutexLocker locker(&mutex); + if (path.isEmpty() || (persistentLogEnabled && persistentLogPath == path)) { + return; + } + + QDir().mkpath(QFileInfo(path).absolutePath()); + persistentLogPath = path; + openPersistentLog(path); +} + +void Logger::prunePersistentLog(const QString &path) +{ + QFileInfo info(path); + if (!info.exists() || info.size() <= LOGGER_PERSISTENT_MAX_BYTES) { + return; + } + + QFile file(path); + if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) { + return; + } + QList allLines; + QTextStream in(&file); + while (!in.atEnd()) { + allLines.append(in.readLine()); + } + file.close(); + + const int start = + allLines.size() > LOGGER_PERSISTENT_KEEP_LINES ? allLines.size() - LOGGER_PERSISTENT_KEEP_LINES : 0; + if (!file.open(QIODevice::WriteOnly | QIODevice::Truncate | QIODevice::Text)) { + return; + } + QTextStream out(&file); + for (int i = start; i < allLines.size(); ++i) { + out << allLines.at(i) << Qt::endl; + } + file.close(); +} + +void Logger::openPersistentLog(const QString &path) +{ + prunePersistentLog(path); + + persistentFileHandle.setFileName(path); + if (!persistentFileHandle.open(QIODevice::WriteOnly | QIODevice::Append | QIODevice::Text)) { + qWarning() << "Logger failed to open persistent log" << path << "for writing"; + return; + } + persistentFileStream.setDevice(&persistentFileHandle); + persistentFileStream << LOGGER_SESSION_MARKER << " " << QDateTime::currentDateTime().toString() << Qt::endl; + persistentFileStream << getClientVersion() << Qt::endl; + persistentFileStream << getSystemArchitecture() << Qt::endl; + persistentFileStream << getClientInstallInfo() << Qt::endl; + persistentFileStream.flush(); + persistentLogEnabled = true; +} + +void Logger::closePersistentLogSession() +{ + if (!persistentLogEnabled) { + return; + } + + persistentLogEnabled = false; + persistentFileStream << "Log session closed at " << QDateTime::currentDateTime().toString() << Qt::endl; + persistentFileStream.flush(); + persistentFileHandle.close(); +} + +QList Logger::getPreviousSessionTail(int maxLines) const +{ + QString path; + { + QMutexLocker locker(&mutex); + path = persistentLogPath; + } + if (path.isEmpty()) { + return {}; + } + return extractPreviousSessionTail(path, maxLines); +} + +QList Logger::extractPreviousSessionTail(const QString &filePath, int maxLines) +{ + QList result; + if (maxLines <= 0) { + return result; + } + + QFile file(filePath); + if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) { + return result; + } + QList allLines; + QTextStream in(&file); + while (!in.atEnd()) { + allLines.append(in.readLine()); + } + file.close(); + + QList markerIndexes; + for (int i = 0; i < allLines.size(); ++i) { + if (allLines.at(i).contains(QLatin1String(LOGGER_SESSION_MARKER))) { + markerIndexes.append(i); + } + } + + QList tail; + if (markerIndexes.isEmpty()) { + tail = allLines; + } else if (markerIndexes.size() == 1) { + // Only the current session has been recorded; there is no previous session yet. + return result; + } else { + const int previousStart = markerIndexes.at(markerIndexes.size() - 2); + const int currentStart = markerIndexes.last(); + for (int i = previousStart; i < currentStart; ++i) { + tail.append(allLines.at(i)); + } + } + + const int start = tail.size() > maxLines ? tail.size() - maxLines : 0; + for (int i = start; i < tail.size(); ++i) { + result.append(tail.at(i)); + } + return result; +} + void Logger::log(QtMsgType /* type */, const QMessageLogContext & /* ctx */, const QString &message) { QMetaObject::invokeMethod(this, "internalLog", Qt::QueuedConnection, Q_ARG(const QString &, message)); @@ -102,6 +239,11 @@ void Logger::internalLog(const QString &message) if (logToFileEnabled) { fileStream << message << Qt::endl; // Print to fileStream } + + if (persistentLogEnabled) { + persistentFileStream << message << Qt::endl; + persistentFileStream.flush(); + } } QString Logger::getSystemArchitecture() diff --git a/cockatrice/src/interface/logger.h b/cockatrice/src/interface/logger.h index 2287da804..cc7cee046 100644 --- a/cockatrice/src/interface/logger.h +++ b/cockatrice/src/interface/logger.h @@ -49,6 +49,9 @@ public: { return logBuffer; } + void setPersistentLogPath(const QString &path); + QList getPreviousSessionTail(int maxLines = 60) const; + static QList extractPreviousSessionTail(const QString &filePath, int maxLines = 60); private: Logger(); @@ -61,11 +64,18 @@ private: QTextStream fileStream; QFile fileHandle; QList logBuffer; - QMutex mutex; + mutable QMutex mutex; + QString persistentLogPath; + bool persistentLogEnabled; + QTextStream persistentFileStream; + QFile persistentFileHandle; protected: void openLogfileSession(); void closeLogfileSession(); + void openPersistentLog(const QString &path); + void prunePersistentLog(const QString &path); + void closePersistentLogSession(); protected slots: void internalLog(const QString &message); diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.cpp new file mode 100644 index 000000000..7fc69e12b --- /dev/null +++ b/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.cpp @@ -0,0 +1,152 @@ +#include "dlg_crash_report.h" + +#include "../../../client/crash/crash_reporter.h" +#include "../../../client/settings/cache_settings.h" +#include "../../logger.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +DlgCrashReport::DlgCrashReport(QWidget *parent) : QDialog(parent) +{ + infoLabel = new QLabel(this); + infoLabel->setWordWrap(true); + + accountLabel = new QLabel(this); + accountLabel->setWordWrap(true); + + previewArea = new QPlainTextEdit(this); + previewArea->setReadOnly(true); + previewArea->setPlainText(buildRedactedPreview()); + + rememberBox = new QCheckBox(this); + + sendButton = new QPushButton(this); + sendButton->setDefault(true); + connect(sendButton, &QPushButton::clicked, this, &DlgCrashReport::actSend); + + dontSendButton = new QPushButton(this); + dontSendButton->setAutoDefault(false); + connect(dontSendButton, &QPushButton::clicked, this, &DlgCrashReport::actDontSend); + + auto *buttonLayout = new QHBoxLayout; + buttonLayout->addWidget(rememberBox); + buttonLayout->addStretch(); + buttonLayout->addWidget(dontSendButton); + buttonLayout->addWidget(sendButton); + + auto *mainLayout = new QVBoxLayout; + mainLayout->addWidget(infoLabel); + mainLayout->addWidget(accountLabel); + mainLayout->addWidget(previewArea); + mainLayout->addLayout(buttonLayout); + setLayout(mainLayout); + + resize(700, 500); + retranslateUi(); +} + +void DlgCrashReport::changeEvent(QEvent *event) +{ + if (event->type() == QEvent::LanguageChange) { + retranslateUi(); + } + QDialog::changeEvent(event); +} + +void DlgCrashReport::retranslateUi() +{ + setWindowTitle(tr("Send crash report")); + infoLabel->setText( + tr("Cockatrice closed unexpectedly. The preview below shows what a crash report would contain (mock).")); + accountLabel->setText(tr("No account needed to send a crash report.")); + rememberBox->setText(tr("Remember my choice")); + sendButton->setText(tr("Send")); + dontSendButton->setText(tr("Don't send")); +} + +QString DlgCrashReport::buildRedactedPreview() +{ + Logger &logger = Logger::getInstance(); + + QStringList header; + header.append(logger.getClientVersion()); + header.append(logger.getSystemArchitecture()); + header.append(logger.getSystemLocale()); + header.append(logger.getClientInstallInfo()); + header.append(QStringLiteral("---")); + + QList logLines = logger.getPreviousSessionTail(); + if (logLines.isEmpty()) { + logLines = logger.getLogBuffer(); + } + const int previewLines = 60; + int start = logLines.size() > previewLines ? logLines.size() - previewLines : 0; + + QStringList lines = header; + for (int i = start; i < logLines.size(); ++i) { + lines.append(CrashReporter::redactCockatriceUrls(CrashReporter::sanitizeLogLine(logLines.at(i)))); + } + return lines.join(QStringLiteral("\n")); +} + +QString DlgCrashReport::writeMockReport(const QString &previewText) +{ + Logger &logger = Logger::getInstance(); + + QJsonObject report; + report[QStringLiteral("mock")] = true; + report[QStringLiteral("timestamp")] = QDateTime::currentDateTimeUtc().toString(Qt::ISODate); + report[QStringLiteral("clientVersion")] = logger.getClientVersion(); + report[QStringLiteral("systemArchitecture")] = logger.getSystemArchitecture(); + report[QStringLiteral("systemLocale")] = logger.getSystemLocale(); + report[QStringLiteral("installInfo")] = logger.getClientInstallInfo(); + report[QStringLiteral("logPreview")] = previewText; + + QString fileName = QStringLiteral("crash-report-mock-%1.json") + .arg(QDateTime::currentDateTimeUtc().toString(QStringLiteral("yyyyMMdd-HHmmss"))); + QString filePath = QDir::tempPath() + QStringLiteral("/") + fileName; + + QFile file(filePath); + if (!file.open(QIODevice::WriteOnly | QIODevice::Text)) { + return QString(); + } + file.write(QJsonDocument(report).toJson()); + file.close(); + return filePath; +} + +void DlgCrashReport::actSend() +{ + if (rememberBox->isChecked()) { + SettingsCache::instance().personal().setCrashReportingMode(PersonalSettings::CrashReportingMode::AlwaysSend); + } + const QString savedPath = writeMockReport(previewArea->toPlainText()); + if (savedPath.isEmpty()) { + qCWarning(CrashReportLog) << "Could not save the mock crash report locally"; + } else { + qCInfo(CrashReportLog) << "Mock crash report saved locally to" << savedPath; + } + accept(); +} + +void DlgCrashReport::actDontSend() +{ + if (rememberBox->isChecked()) { + SettingsCache::instance().personal().setCrashReportingMode(PersonalSettings::CrashReportingMode::NeverSend); + } + reject(); +} diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.h b/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.h new file mode 100644 index 000000000..576eb3cf9 --- /dev/null +++ b/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.h @@ -0,0 +1,51 @@ +/** + * @file dlg_crash_report.h + * @ingroup Dialogs + */ + +#ifndef DLG_CRASH_REPORT_H +#define DLG_CRASH_REPORT_H + +#include +#include +#include + +class QCheckBox; +class QLabel; +class QPlainTextEdit; +class QPushButton; + +/** + * @brief MOCK crash-report dialog (UI + settings only, no network). + * + * The Send path writes a redacted JSON file to the temp dir instead of + * uploading anything. Real Sentry upload comes in a later phase. + */ +class DlgCrashReport : public QDialog +{ + Q_OBJECT +public: + explicit DlgCrashReport(QWidget *parent = nullptr); + + static QString buildRedactedPreview(); + static QString writeMockReport(const QString &previewText); + +protected: + void changeEvent(QEvent *event) override; + +private: + QLabel *infoLabel; + QLabel *accountLabel; + QPlainTextEdit *previewArea; + QCheckBox *rememberBox; + QPushButton *sendButton; + QPushButton *dontSendButton; + + void retranslateUi(); + +private slots: + void actSend(); + void actDontSend(); +}; + +#endif diff --git a/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp b/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp index fa8938b32..a19417661 100644 --- a/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp +++ b/cockatrice/src/interface/widgets/settings_page/general_settings_page.cpp @@ -133,6 +133,27 @@ GeneralSettingsPage::GeneralSettingsPage() versionGroupBox = new QGroupBox; versionGroupBox->setLayout(versionGrid); + // crash reporting consent (stored in personal.ini, not debug.ini) + crashReportingBox.addItem(""); // texts set in retranslateUi + crashReportingBox.addItem(""); + crashReportingBox.addItem(""); + crashReportingBox.setCurrentIndex(static_cast(settings.personal().getCrashReportingMode())); + + connect(&crashReportingBox, qOverload(&QComboBox::currentIndexChanged), this, [](int index) { + if (index < 0 || index > static_cast(PersonalSettings::CrashReportingMode::NeverSend)) { + return; + } + SettingsCache::instance().personal().setCrashReportingMode( + static_cast(index)); + }); + + auto *crashReportingGrid = new QGridLayout; + crashReportingGrid->addWidget(&crashReportingLabel, 0, 0); + crashReportingGrid->addWidget(&crashReportingBox, 0, 1); + + crashReportingGroupBox = new QGroupBox; + crashReportingGroupBox->setLayout(crashReportingGrid); + // card database settings startupCardUpdateCheckBehaviorSelector.addItem(""); // these will be set in retranslateUI startupCardUpdateCheckBehaviorSelector.addItem(""); @@ -336,6 +357,7 @@ GeneralSettingsPage::GeneralSettingsPage() auto *mainLayout = new QVBoxLayout; mainLayout->addWidget(languageGroupBox); mainLayout->addWidget(versionGroupBox); + mainLayout->addWidget(crashReportingGroupBox); mainLayout->addWidget(cardDatabaseGroupBox); mainLayout->addWidget(startupGroupBox); mainLayout->addWidget(pathsGroupBox); @@ -569,13 +591,19 @@ void GeneralSettingsPage::retranslateUi() tr("English and selected card language")); advertiseTranslationPageLabel.setText( QString("%2").arg(WIKI_TRANSLATION_FAQ).arg(tr("How to help with translations"))); - versionGroupBox->setTitle(tr("Version settings")); updateReleaseChannelLabel.setText(tr("Update channel")); startupUpdateCheckCheckBox.setText(tr("Check for client updates on startup")); updateNotificationCheckBox.setText(tr("Notify if a feature supported by the server is missing in my client")); newVersionOracleCheckBox.setText(tr("Automatically run Oracle when running a new version of Cockatrice")); + crashReportingGroupBox->setTitle(tr("Crash reporting")); + crashReportingLabel.setText(tr("Send crash reports:")); + crashReportingBox.setItemText(static_cast(PersonalSettings::CrashReportingMode::Ask), tr("Ask every time")); + crashReportingBox.setItemText(static_cast(PersonalSettings::CrashReportingMode::AlwaysSend), + tr("Always send")); + crashReportingBox.setItemText(static_cast(PersonalSettings::CrashReportingMode::NeverSend), tr("Never send")); + // We can't change the strings after they're put into the QComboBox, so this is our workaround int oldIndex = updateReleaseChannelBox.currentIndex(); updateReleaseChannelBox.clear(); diff --git a/cockatrice/src/interface/widgets/settings_page/general_settings_page.h b/cockatrice/src/interface/widgets/settings_page/general_settings_page.h index fcf1c78d0..20564c968 100644 --- a/cockatrice/src/interface/widgets/settings_page/general_settings_page.h +++ b/cockatrice/src/interface/widgets/settings_page/general_settings_page.h @@ -49,6 +49,7 @@ private slots: private: QGroupBox *languageGroupBox; QGroupBox *versionGroupBox; + QGroupBox *crashReportingGroupBox; QGroupBox *cardDatabaseGroupBox; QGroupBox *startupGroupBox; QGroupBox *pathsGroupBox; @@ -70,6 +71,9 @@ private: QCheckBox updateNotificationCheckBox; QCheckBox newVersionOracleCheckBox; + QLabel crashReportingLabel; + QComboBox crashReportingBox; + QLabel startupCardUpdateCheckBehaviorLabel; QComboBox startupCardUpdateCheckBehaviorSelector; QLabel cardUpdateCheckIntervalLabel; diff --git a/cockatrice/src/main.cpp b/cockatrice/src/main.cpp index 3f63efc3b..38ec3d137 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -20,6 +20,7 @@ #include "main.h" +#include "client/crash/crash_reporter.h" #include "client/network/update/card_spoiler/spoiler_background_updater.h" #include "client/settings/cache_settings.h" #include "client/sound_engine.h" @@ -30,6 +31,7 @@ #include "interface/logger.h" #include "interface/pixel_map_generator.h" #include "interface/theme_manager.h" +#include "interface/widgets/dialogs/dlg_crash_report.h" #include "interface/window_main.h" #include "libcockatrice/card/database/card_database.h" #include "libcockatrice/rng/rng_abstract.h" @@ -44,6 +46,9 @@ #include #include #include +#include +#include +#include #include #include #include @@ -259,6 +264,46 @@ int main(int argc, char *argv[]) qApp->setAttribute(Qt::AA_DontShowIconsInMenus, true); #endif + // Crash reporting (increment 1): init the SDK early so a crash anywhere below is captured into the + // local database. Uploads stay consent-gated (require_user_consent, armed in CrashReporter::init): + // AlwaysSend gives consent here so cached envelopes flush at startup; any other mode starts + // unconsented and the dialog below drives give/revoke. An empty DSN skips the SDK entirely and + // the mock fallback further below behaves exactly as before. + CrashReporter crashReporter; + const QString crashDatabasePath = SettingsCache::instance().getSettingsPath() + QStringLiteral("sentry"); + { + QString crashDsn = QString::fromUtf8(qgetenv("COCKATRICE_SENTRY_DSN")); + if (crashDsn.isEmpty()) { + crashDsn = QString::fromLatin1(COCKATRICE_SENTRY_DSN); + } + crashReporter.init(crashDsn, QString::fromLatin1(VERSION_STRING), crashDatabasePath, + SettingsCache::instance().personal().getCrashReportingMode()); + if (crashReporter.isReady() && SettingsCache::instance().personal().getCrashReportingMode() == + PersonalSettings::CrashReportingMode::AlwaysSend) { + // Silent opt-in: cached crash envelopes flush automatically once consent is given. + crashReporter.giveConsent(); + } + // Mid-session mode changes must reach the SDK: without this, switching from AlwaysSend to + // NeverSend (or Ask) leaves consent given until restart, so a later crash in this session + // would still upload after the user opted out. crashReporter outlives main(), so capturing + // it here is safe. + QObject::connect(&SettingsCache::instance().personal(), &PersonalSettings::crashReportingModeChanged, + [&crashReporter](PersonalSettings::CrashReportingMode mode) { + if (mode == PersonalSettings::CrashReportingMode::AlwaysSend) { + crashReporter.giveConsent(); + } else { + crashReporter.revokeConsent(); + } + }); + } + + // Persistent session log: keeps this run's logs on disk so the next-startup crash dialog can + // attach the crashed session's tail (the in-memory buffer is lost on crash). Opened in Append + // mode with a per-launch session marker; read back via Logger::getPreviousSessionTail(). + const QString persistentLogPath = + SettingsCache::instance().getSettingsPath() + QStringLiteral("logs/cockatrice.log"); + Logger::getInstance().setPersistentLogPath(persistentLogPath); + // Translations #ifdef Q_OS_MAC translationPath = qApp->applicationDirPath() + "/../Resources/translations"; @@ -276,7 +321,9 @@ int main(int argc, char *argv[]) parser.addOptions( {{{"c", "connect"}, QCoreApplication::translate("main", "Connect on startup"), "user:pass@host:port"}, - {{"d", "debug-output"}, QCoreApplication::translate("main", "Debug to file")}}); + {{"d", "debug-output"}, QCoreApplication::translate("main", "Debug to file")}, + {"simulate-crash", + QCoreApplication::translate("main", "Show the mock crash-report dialog (nothing is sent)")}}); parser.process(app); @@ -413,6 +460,51 @@ int main(int argc, char *argv[]) ui.show(); qCInfo(MainLog) << "ui.show() finished"; + // MOCK crash-report flow (UI + settings only): no sentry-native upload, no network. + // Triggered by --simulate-crash or by leftover Windows crash dumps. Handled dumps are deleted + // after Send / Don't send (or a silent NeverSend skip) so one crash never prompts twice; on + // non-Windows nothing writes cockatrice.crash.*.dmp yet, so --simulate-crash is the trigger there. + { + QString crashDumpDir; +#ifdef Q_OS_WIN + // Mirrors the writer (CockatriceUnhandledExceptionFilter): it appends CrashDumps/Cockatrice + // to %LOCALAPPDATA%, or, when its known-folder lookup fails, to the temp directory walked up + // two parent_path() steps. %LOCALAPPDATA% is this scan's stand-in for FOLDERID_LocalAppData: + // both come from the same profile, so when the lookup fails the variable is normally unset. + QString crashDumpRootPath = QString::fromLocal8Bit(qgetenv("LOCALAPPDATA")); + if (crashDumpRootPath.isEmpty()) { + // The writer's fallback expression, verbatim — QDir::cdUp() is existence-checked and + // would silently leave the path alone here, parent_path() is not. + const std::filesystem::path fallback = std::filesystem::temp_directory_path().parent_path().parent_path(); + crashDumpRootPath = QString::fromStdWString(fallback.wstring()); + } + crashDumpDir = QDir(crashDumpRootPath).absoluteFilePath(QStringLiteral("CrashDumps/Cockatrice")); +#else + crashDumpDir = QDir::tempPath(); +#endif + const QStringList crashDumps = + QDir(crashDumpDir).entryList({QStringLiteral("cockatrice.crash.*.dmp")}, QDir::Files); + if (parser.isSet("simulate-crash") || !crashDumps.isEmpty()) { + const PersonalSettings::CrashReportingMode crashMode = + SettingsCache::instance().personal().getCrashReportingMode(); + if (crashMode == PersonalSettings::CrashReportingMode::AlwaysSend) { + crashReporter.giveConsent(); + DlgCrashReport::writeMockReport(DlgCrashReport::buildRedactedPreview()); + } else if (crashMode == PersonalSettings::CrashReportingMode::Ask) { + DlgCrashReport crashDialog(&ui); + if (crashDialog.exec() == QDialog::Accepted) { + crashReporter.giveConsent(); + } else { + crashReporter.revokeConsent(); + } + } + // NeverSend: skip silently (no dialog, nothing written or sent). + for (const QString &dump : crashDumps) { + QFile::remove(crashDumpDir + QStringLiteral("/") + dump); + } + } + } + // force shortcuts to be shown/hidden in right-click menus, regardless of system defaults qApp->setAttribute(Qt::AA_DontShowShortcutsInContextMenus, !SettingsCache::instance().userInterface().getShowShortcuts()); @@ -437,6 +529,8 @@ int main(int argc, char *argv[]) int ret = app.exec(); + crashReporter.shutdown(); + qCInfo(MainLog) << "Event loop finished, terminating..."; delete rng; PingPixmapGenerator::clear(); diff --git a/libcockatrice_settings/libcockatrice/settings/debug_settings.cpp b/libcockatrice_settings/libcockatrice/settings/debug_settings.cpp index c36e7187e..709155b1c 100644 --- a/libcockatrice_settings/libcockatrice/settings/debug_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/debug_settings.cpp @@ -34,4 +34,4 @@ int DebugSettings::getLocalGamePlayerCount() const QString DebugSettings::getDeckPathForPlayer(const QString &playerName) const { return getValue(playerName, "localgame", "deck").toString(); -} \ No newline at end of file +} diff --git a/libcockatrice_settings/libcockatrice/settings/personal_settings.cpp b/libcockatrice_settings/libcockatrice/settings/personal_settings.cpp index 682144485..79d06ddd4 100644 --- a/libcockatrice_settings/libcockatrice/settings/personal_settings.cpp +++ b/libcockatrice_settings/libcockatrice/settings/personal_settings.cpp @@ -51,3 +51,24 @@ void PersonalSettings::setSeenTips(const QList &_seenTips) } setValue(QVariant::fromValue(storedTipList), "seenTips", "tipOfDay"); } + +PersonalSettings::CrashReportingMode PersonalSettings::getCrashReportingMode() const +{ + const int raw = getValue("crashReportingMode", QString(), QString(), 0).toInt(); + switch (raw) { + case 1: + return CrashReportingMode::AlwaysSend; + case 2: + return CrashReportingMode::NeverSend; + case 0: + default: + // Clamp corrupt ini values back to Ask; the QSettings key string is unchanged (no migration). + return CrashReportingMode::Ask; + } +} + +void PersonalSettings::setCrashReportingMode(CrashReportingMode _crashReportingMode) +{ + setValue(static_cast(_crashReportingMode), "crashReportingMode"); + emit crashReportingModeChanged(_crashReportingMode); +} diff --git a/libcockatrice_settings/libcockatrice/settings/personal_settings.h b/libcockatrice_settings/libcockatrice/settings/personal_settings.h index 0819af41b..0cd51cdd0 100644 --- a/libcockatrice_settings/libcockatrice/settings/personal_settings.h +++ b/libcockatrice_settings/libcockatrice/settings/personal_settings.h @@ -24,8 +24,19 @@ public: void setShowTipsOnStartup(bool _showTipsOnStartup); void setSeenTips(const QList &_seenTips); + enum class CrashReportingMode + { + Ask = 0, ///< Show the crash-report dialog after an unclean exit. + AlwaysSend = 1, ///< Upload crash reports without asking. + NeverSend = 2 ///< Never upload; stay silent. + }; + + [[nodiscard]] CrashReportingMode getCrashReportingMode() const; + void setCrashReportingMode(CrashReportingMode _crashReportingMode); + signals: void langChanged(); + void crashReportingModeChanged(CrashReportingMode mode); public: explicit PersonalSettings(const QString &settingPath, QObject *parent = nullptr); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8811b8623..aac4d4818 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -422,6 +422,21 @@ if(WITH_CLIENT) ${CMAKE_SOURCE_DIR}/cockatrice/src ${CMAKE_SOURCE_DIR}/libcockatrice_utility ) + # CrashReporter is client code; DEFINITIONS keeps the test hermetic by compiling the + # no-op path (no sentry-native headers needed) regardless of WITH_CRASH_REPORTING. + cockatrice_add_test( + NAME + crash_reporter_test + SOURCES + ${CMAKE_SOURCE_DIR}/cockatrice/src/client/crash/crash_reporter.cpp + crash_reporter_test.cpp + LIBS + libcockatrice_settings + INCLUDE_DIRS + ${CMAKE_SOURCE_DIR}/cockatrice/src + DEFINITIONS + COCKATRICE_WITH_CRASH_REPORTING=0 + ) endif() add_subdirectory(card_zone_algorithms) diff --git a/tests/crash_reporter_test.cpp b/tests/crash_reporter_test.cpp new file mode 100644 index 000000000..c52a2e495 --- /dev/null +++ b/tests/crash_reporter_test.cpp @@ -0,0 +1,87 @@ +#include "client/crash/crash_reporter.h" + +#include +#include +#include +#include + +namespace +{ + +TEST(CrashReporterTest, SanitizeLogLineStripsAnsiColorCodes) +{ + ASSERT_EQ(CrashReporter::sanitizeLogLine(QStringLiteral("\033[32mhello\033[0m")), QStringLiteral("hello")); + ASSERT_EQ(CrashReporter::sanitizeLogLine(QStringLiteral("\033[1;31mfatal\033[0m")), QStringLiteral("fatal")); + ASSERT_EQ(CrashReporter::sanitizeLogLine(QStringLiteral("plain line")), QStringLiteral("plain line")); +} + +TEST(CrashReporterTest, RedactCockatriceUrlsKeepsSchemeAndHostOnly) +{ + ASSERT_EQ( + CrashReporter::redactCockatriceUrls(QStringLiteral("open cockatrice://opendeck?token=secret&user=me now")), + QStringLiteral("open cockatrice://opendeck now")); +} + +TEST(CrashReporterTest, RedactCockatriceUrlsIsIdempotent) +{ + // A secret-free URL must not match again after redaction (regression: this hung forever). + ASSERT_EQ(CrashReporter::redactCockatriceUrls(QStringLiteral("open cockatrice://opendeck now")), + QStringLiteral("open cockatrice://opendeck now")); +} + +TEST(CrashReporterTest, RedactCockatriceUrlsLeavesOtherTextAlone) +{ + ASSERT_EQ(CrashReporter::redactCockatriceUrls(QStringLiteral("https://cockatrice.github.io/?token=secret")), + QStringLiteral("https://cockatrice.github.io/?token=secret")); + ASSERT_EQ(CrashReporter::sanitizeLogLine(QStringLiteral("")), QStringLiteral("")); +} + +TEST(CrashReporterTest, InitSkippedWithoutDsn) +{ + CrashReporter reporter; + reporter.init(QString(), QStringLiteral("3.1.2"), QStringLiteral("/tmp/cockatrice-crash-test"), + PersonalSettings::CrashReportingMode::Ask); + ASSERT_FALSE(reporter.isReady()); + reporter.shutdown(); + ASSERT_FALSE(reporter.isReady()); +} + +TEST(CrashReporterTest, InitSkippedWhenNeverSend) +{ + CrashReporter reporter; + reporter.init(QStringLiteral("https://key@sentry.io/1"), QStringLiteral("3.1.2"), + QStringLiteral("/tmp/cockatrice-crash-test"), PersonalSettings::CrashReportingMode::NeverSend); + ASSERT_FALSE(reporter.isReady()); + reporter.shutdown(); +} + +// The init() tests above compile to no-ops when the SDK is off, so they pass +// regardless of the gating; the gating decision itself is covered here, in both +// build modes, through the SDK-independent helper init() consults. +TEST(CrashReporterTest, ReportingEnabledGating) +{ + using Mode = PersonalSettings::CrashReportingMode; + const QString dsn = QStringLiteral("https://key@sentry.io/1"); + ASSERT_FALSE(CrashReporter::isReportingEnabled(QString(), Mode::Ask)); + ASSERT_FALSE(CrashReporter::isReportingEnabled(QString(), Mode::AlwaysSend)); + ASSERT_FALSE(CrashReporter::isReportingEnabled(QString(), Mode::NeverSend)); + ASSERT_TRUE(CrashReporter::isReportingEnabled(dsn, Mode::Ask)); + ASSERT_TRUE(CrashReporter::isReportingEnabled(dsn, Mode::AlwaysSend)); + ASSERT_FALSE(CrashReporter::isReportingEnabled(dsn, Mode::NeverSend)); +} + +TEST(CrashReporterTest, ShutdownWithoutInitIsSafe) +{ + CrashReporter reporter; + ASSERT_FALSE(reporter.isReady()); + reporter.shutdown(); + ASSERT_FALSE(reporter.isReady()); +} + +} // namespace + +int main(int argc, char **argv) +{ + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tests/settings/settings_defaults_test.cpp b/tests/settings/settings_defaults_test.cpp index 0d4cdbafe..aea7f1d54 100644 --- a/tests/settings/settings_defaults_test.cpp +++ b/tests/settings/settings_defaults_test.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -367,6 +368,32 @@ TEST_F(SettingsDefaultsTest, Personal_ShowTipsOnStartup_Default) ASSERT_EQ(s.getShowTipsOnStartup(), true); } +TEST_F(SettingsDefaultsTest, Personal_CrashReportingMode_Default) +{ + PersonalSettings s(settingsPath, nullptr); + ASSERT_EQ(s.getCrashReportingMode(), PersonalSettings::CrashReportingMode::Ask); +} + +TEST_F(SettingsDefaultsTest, Personal_CrashReportingMode_SetAndGet) +{ + PersonalSettings s(settingsPath, nullptr); + s.setCrashReportingMode(PersonalSettings::CrashReportingMode::AlwaysSend); + ASSERT_EQ(s.getCrashReportingMode(), PersonalSettings::CrashReportingMode::AlwaysSend); + s.setCrashReportingMode(PersonalSettings::CrashReportingMode::NeverSend); + ASSERT_EQ(s.getCrashReportingMode(), PersonalSettings::CrashReportingMode::NeverSend); + s.setCrashReportingMode(PersonalSettings::CrashReportingMode::Ask); + ASSERT_EQ(s.getCrashReportingMode(), PersonalSettings::CrashReportingMode::Ask); +} + +TEST_F(SettingsDefaultsTest, Personal_CrashReportingMode_ClampsCorruptValue) +{ + QSettings raw(settingsPath + "personal.ini", QSettings::IniFormat); + raw.setValue("personal/crashReportingMode", 99); + raw.sync(); + PersonalSettings s(settingsPath, nullptr); + ASSERT_EQ(s.getCrashReportingMode(), PersonalSettings::CrashReportingMode::Ask); +} + // --- DownloadSettings --- TEST_F(SettingsDefaultsTest, Download_PicDownload_Default) From ad0fd2d4b04662c259030d275f92f170bbe356eb Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 19 Oct 2026 14:57:03 +0200 Subject: [PATCH 02/90] [CrashReporting] Sentry send path with consent gating (#7522) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [CrashReporting] Real send path with consent gating (DSN still placeholder) require_user_consent + give/revoke around the Ask dialog, message event with in-memory crash-log.txt bytes attachment, before_send scrubs message/logentry/breadcrumbs/data of cockatrice:// secrets. Failure box offers Save (shows path) / Discard (revokes + rejects) / Cancel (back to dialog); reject() override so X/Escape cannot bypass revocation; silent AlwaysSend path falls back to a local file. Empty DSN keeps the previous mock behavior byte-identical. Tests: 528/528 ctest green, format clean, code + UI gates PASS. * [CrashReporting] Attach crash log and user note to the pending crash event The redacted log used to ride a separate message event from the native crash (minidump + stack), forcing manual correlation in Sentry. Fold it into the pending crash envelope(s) instead by appending well-formed attachment items before consent releases them: sentry-native replays cached envelopes byte-for-byte (raw read, raw serialize in 0.17.3), so the appended crash-log.txt and user-note.txt items upload with the very event that already carries the dump and stack. Covers both storage locations: Ask mode's consent-held files in /cache and AlwaysSend's transport-dumped files in /.run, which must be attached before sentry_init replays them. When no pending crash event exists (unclean exit without a native crash) the standalone message event remains as fallback, now also carrying the optional note. The dialog gains an optional free-text field ('Here's what I was doing...') that travels as user-note.txt; the mock JSON records it too. * [CrashReporting] Send PROJECT_VERSION as the Sentry release main() reported VERSION_STRING (PROJECT_VERSION_FRIENDLY) as the Sentry release — that carries a build date, so every build of the same version landed in its own release, and it never matched the installer, which reports CPACK/PROJECT_VERSION. Bake COCKATRICE_SENTRY_RELEASE= PROJECT_VERSION instead: date-free, identical between client and installer, and derived from the git tag on tag builds, so events, symbol uploads and the installer log all group under one release. * [CrashReporting] Friendlier crash dialog copy Reword the report dialog along a single friendly arc: headline ('Oops, we crashed — and now we need your help to make Cockatrice better!'), why helping is easy, exactly what gets collected (crash dump with stack trace, app version, OS info, redacted log excerpt), the value of each report ('better and less crashy for everyone'), and the consent choice ('Send and remember makes this automatic without interruption in the future'). The checkbox itself stays direction-neutral ('Remember my choice') because it also persists a 'Don't send' decision. * [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. --------- Co-authored-by: Lukas Brübach --- cmake/CrashReporting.cmake | 7 + .../src/client/crash/crash_reporter.cpp | 409 ++++++++++++++++++ cockatrice/src/client/crash/crash_reporter.h | 97 ++++- .../widgets/dialogs/dlg_crash_report.cpp | 107 ++++- .../widgets/dialogs/dlg_crash_report.h | 22 +- cockatrice/src/main.cpp | 105 +++-- tests/crash_reporter_test.cpp | 265 +++++++++++- 7 files changed, 965 insertions(+), 47 deletions(-) diff --git a/cmake/CrashReporting.cmake b/cmake/CrashReporting.cmake index a728e25c2..abd029cc2 100644 --- a/cmake/CrashReporting.cmake +++ b/cmake/CrashReporting.cmake @@ -32,6 +32,12 @@ function(cockatrice_target_enable_crash_reporting _target) endif() # Always baked in (possibly empty): main() prefers the COCKATRICE_SENTRY_DSN env var at runtime. target_compile_definitions(${_target} PRIVATE "COCKATRICE_SENTRY_DSN=\"${COCKATRICE_SENTRY_DSN}\"") + # Sentry release for events from this target: PROJECT_VERSION (getversion.cmake ran before this + # file was included). Deliberately not PROJECT_VERSION_FRIENDLY / VERSION_STRING — that carries + # a build date, so every build of the same version would land in a different Sentry release. + # PROJECT_VERSION matches what the installer reports in its own log and what CPack derives the + # package version from, so client and installer events group under one release. + target_compile_definitions(${_target} PRIVATE "COCKATRICE_SENTRY_RELEASE=\"${PROJECT_VERSION}\"") endfunction() if(WITH_CRASH_REPORTING AND WITH_CLIENT) @@ -77,6 +83,7 @@ if(WITH_CRASH_REPORTING AND WITH_CLIENT) sentry GIT_REPOSITORY https://github.com/getsentry/sentry-native.git GIT_TAG ${COCKATRICE_SENTRY_TAG} + GIT_SHALLOW TRUE ) fetchcontent_makeavailable(sentry) diff --git a/cockatrice/src/client/crash/crash_reporter.cpp b/cockatrice/src/client/crash/crash_reporter.cpp index fe1678b11..720bcd350 100644 --- a/cockatrice/src/client/crash/crash_reporter.cpp +++ b/cockatrice/src/client/crash/crash_reporter.cpp @@ -4,15 +4,216 @@ #include #endif +#include #include #include +#include +#include #include #include #include +#include #include +#include Q_LOGGING_CATEGORY(CrashReportLog, "crash_report"); +#if COCKATRICE_WITH_CRASH_REPORTING +namespace +{ + +constexpr char CockatriceUrlScheme[] = "cockatrice://"; + +/** + * @brief Qt-free cockatrice:// secret scrubber for the before_send hook. + * + * before_send does not run in async-signal context on our backends (breakpad invokes it on the + * crashing thread outside the signal handler, and crashpad/native crashes bypass the hook + * entirely), but it still avoids QString/QRegularExpression so it stays safe to call from the + * crash-handler thread. Truncates each URL at the first terminator, keeping scheme://host like + * redactCockatriceUrls does. (std::string allocates, so this is not strictly signal-safe.) + */ +std::string scrubCockatriceUrls(const char *input) +{ + std::string out; + const char *pos = input; + const char *scheme = CockatriceUrlScheme; + const size_t schemeLen = std::strlen(scheme); + while (*pos != '\0') { + const char *found = std::strstr(pos, scheme); + if (found == nullptr) { + out.append(pos); + break; + } + out.append(pos, found); + const char *hostEnd = found + schemeLen; + while (*hostEnd != '\0' && *hostEnd != '/' && *hostEnd != '?' && *hostEnd != '#' && *hostEnd != ' ' && + *hostEnd != '\t' && *hostEnd != '\n' && *hostEnd != '\'' && *hostEnd != '"' && *hostEnd != '<' && + *hostEnd != '>') { + ++hostEnd; + } + out.append(found, hostEnd); + pos = hostEnd; + } + return out; +} + +void scrubStringValue(sentry_value_t holder, const char *key) +{ + sentry_value_t current = sentry_value_get_by_key(holder, key); + if (sentry_value_get_type(current) != SENTRY_VALUE_TYPE_STRING) { + return; + } + const char *raw = sentry_value_as_string(current); + if (raw == nullptr || std::strstr(raw, CockatriceUrlScheme) == nullptr) { + return; + } + sentry_value_set_by_key(holder, key, sentry_value_new_string(scrubCockatriceUrls(raw).c_str())); +} + +/** + * @brief Scrub trivially reachable string values inside a breadcrumb data object. + */ +int scrubDataEntry(const char *key, sentry_value_t value, void *userdata) +{ + if (sentry_value_get_type(value) != SENTRY_VALUE_TYPE_STRING) { + return 0; + } + const char *raw = sentry_value_as_string(value); + if (raw == nullptr || std::strstr(raw, CockatriceUrlScheme) == nullptr) { + return 0; + } + auto *holder = static_cast(userdata); + sentry_value_set_by_key(*holder, key, sentry_value_new_string(scrubCockatriceUrls(raw).c_str())); + return 0; +} + +void scrubDataObject(sentry_value_t data) +{ + if (sentry_value_get_type(data) != SENTRY_VALUE_TYPE_OBJECT) { + return; + } + sentry_value_foreach_key_value(data, scrubDataEntry, &data); +} + +void scrubCrumbList(sentry_value_t crumbs) +{ + const size_t count = sentry_value_get_length(crumbs); + for (size_t i = 0; i < count; ++i) { + sentry_value_t crumb = sentry_value_get_by_index(crumbs, i); + if (sentry_value_get_type(crumb) != SENTRY_VALUE_TYPE_OBJECT) { + continue; + } + scrubStringValue(crumb, "message"); + scrubDataObject(sentry_value_get_by_key(crumb, "data")); + } +} + +void scrubBreadcrumbs(sentry_value_t event) +{ + // The SDK merges scope breadcrumbs onto the event as a plain list (sentry__value_merge_breadcrumbs + // in sentry_scope.c); the { "values": [...] } envelope shape only exists on the wire. Accept both + // so the scrubbing runs on the actual merged form. + sentry_value_t breadcrumbs = sentry_value_get_by_key(event, "breadcrumbs"); + if (sentry_value_get_type(breadcrumbs) == SENTRY_VALUE_TYPE_LIST) { + scrubCrumbList(breadcrumbs); + return; + } + if (sentry_value_get_type(breadcrumbs) == SENTRY_VALUE_TYPE_OBJECT) { + sentry_value_t values = sentry_value_get_by_key(breadcrumbs, "values"); + if (sentry_value_get_type(values) == SENTRY_VALUE_TYPE_LIST) { + scrubCrumbList(values); + } + } +} + +/** + * @brief before_send hook: scrub cockatrice:// secrets from the event. + * + * Defense in depth — payloads we build are pre-redacted. No breadcrumbs are recorded today (the Qt + * integration stays off), so this currently covers the top-level message and logentry.formatted, + * plus any breadcrumb messages/data that reach the merged event in the future. Native crash + * minidumps bypass this hook regardless of backend: breakpad captures them in-process without + * invoking before_send, and fast-fail crashes on Windows skip the local handler, so before_send is + * never invoked for those. + */ +sentry_value_t scrubEventBeforeSend(sentry_value_t event, sentry_hint_t *hint, void *userData) +{ + Q_UNUSED(hint); + Q_UNUSED(userData); + scrubStringValue(event, "message"); + sentry_value_t logentry = sentry_value_get_by_key(event, "logentry"); + if (sentry_value_get_type(logentry) == SENTRY_VALUE_TYPE_OBJECT) { + scrubStringValue(logentry, "formatted"); + } + scrubBreadcrumbs(event); + return event; +} + +} // 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; @@ -26,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 @@ -46,9 +265,13 @@ void CrashReporter::init(const QString &dsn, // Consent-gated uploads: crashes are captured into the local database but // nothing leaves the machine until giveConsent(). sentry_options_set_require_user_consent(options, 1); + sentry_options_set_before_send(options, scrubEventBeforeSend, nullptr); 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 } @@ -80,12 +303,198 @@ void CrashReporter::giveConsent() void CrashReporter::revokeConsent() { #if COCKATRICE_WITH_CRASH_REPORTING + // Revoke (not reset): an explicit refusal blocks uploads; reset would + // return to unknown, which blocks uploads too but loses the decision. if (ready) { sentry_user_consent_revoke(); } #endif } +bool CrashReporter::sendReport(const QString &redactedPreview, const QString &userNote) +{ +#if COCKATRICE_WITH_CRASH_REPORTING + if (!ready) { + return false; + } + // Local scope (not the global one): the log preview rides only on this event. A global + // sentry_attach_bytes would linger and ride along on every later event in the session, + // including a real native crash later on — and with the crashpad backend byte attachments + // are written to disk for exactly that. In-memory bytes also mean no report files land in + // the Sentry DB dir, so there is no disk growth and no same-second filename collision. + sentry_scope_t *scope = sentry_scope_new(); + if (scope == nullptr) { + qCWarning(CrashReportLog) << "Could not create scope for crash report"; + return false; + } + const QByteArray previewBytes = redactedPreview.toUtf8(); + const sentry_uuid_t attachmentId = sentry_scope_attach_bytes( + scope, previewBytes.constData(), static_cast(previewBytes.size()), "crash-log.txt"); + const bool attachOk = sentry_uuid_is_nil(&attachmentId) == 0; + if (!attachOk) { + qCWarning(CrashReportLog) << "Could not attach crash log to crash report"; + } + bool noteAttachOk = true; + if (!userNote.isEmpty()) { + const QByteArray noteBytes = userNote.toUtf8(); + const sentry_uuid_t noteId = sentry_scope_attach_bytes(scope, noteBytes.constData(), + static_cast(noteBytes.size()), "user-note.txt"); + noteAttachOk = sentry_uuid_is_nil(¬eId) == 0; + if (!noteAttachOk) { + qCWarning(CrashReportLog) << "Could not attach user note to crash report"; + } + } + + // Release is attached automatically from the init options. Deliberately no + // sentry_set_user call: the report must carry no PII. The dialog collects + // no contact info, so the message event + attachments are the whole payload + // (sentry_value_new_feedback/sentry_capture_feedback exist but stay unused). + sentry_value_t event = sentry_value_new_message_event(SENTRY_LEVEL_ERROR, "crash-report", "Crash report"); + const sentry_uuid_t eventId = sentry_scope_capture_event(scope, event, nullptr); + sentry_scope_free(scope); + if (sentry_uuid_is_nil(&eventId)) { + qCWarning(CrashReportLog) << "Failed to capture crash report event"; + return false; + } + char eventIdStr[37] = {}; + sentry_uuid_as_string(&eventId, eventIdStr); + // Capture only queues the envelope to the background transport: a network failure stays + // invisible here, so true means "queued", not "delivered". The Save fallback is offered only + // when the local capture itself fails. + qCInfo(CrashReportLog) << "Crash report queued, event id:" << eventIdStr; + // A failed attachment still captures the event above, but reports failure + // so callers offer the local-save fallback instead of silent success. + return attachOk && noteAttachOk; +#else + Q_UNUSED(redactedPreview); + Q_UNUSED(userNote); + return false; +#endif +} + +bool CrashReporter::appendAttachmentToEnvelope(const QString &envelopePath, + const QString &filename, + const QByteArray &payload) +{ + // QIODevice::ReadWrite creates a missing file; appending to a file that was never + // written by the SDK would fabricate an envelope, so require it to exist. + if (!QFile::exists(envelopePath)) { + return false; + } + QFile file(envelopePath); + if (!file.open(QIODevice::ReadWrite)) { + return false; + } + // Item boundaries are single newlines: make sure the header starts on a fresh line. + if (file.size() > 0) { + if (!file.seek(file.size() - 1)) { + return false; + } + char lastByte = 0; + if (file.read(&lastByte, 1) != 1) { + return false; + } + if (lastByte != '\n' && file.write("\n", 1) != 1) { + return false; + } + } + // Item shape mirrors sentry__envelope_add_attachment in the SDK. `length` is what the + // SDK's envelope re-parser uses to frame the payload (a missing length ends the item at + // the next newline, which would truncate a multi-line log and make the rest of the file + // fail to parse); `attachment_length` is the logical attachment size the SDK also emits. + // The filename is a compile-time ASCII constant from the callers, so no JSON escaping is + // needed. + const QByteArray header = QByteArrayLiteral("{\"type\":\"attachment\",\"length\":") + + QByteArray::number(payload.size()) + QByteArrayLiteral(",\"attachment_length\":") + + QByteArray::number(payload.size()) + QByteArrayLiteral(",\"filename\":\"") + + filename.toUtf8() + QByteArrayLiteral("\",\"content_type\":\"text/plain\"}") + '\n'; + if (file.write(header) != header.size()) { + return false; + } + return file.write(payload) == payload.size(); +} + +QStringList CrashReporter::pendingEnvelopePaths(const QString &databasePath) +{ + static const auto isUuid = [](const QString &id) { return id.size() == 36 && id.count(QLatin1Char('-')) == 4; }; + static const auto isEnvelopeName = [](const QString &name) { + if (name == QStringLiteral("__sentry-crash.envelope")) { + return false; + } + const QString stem = name.left(name.size() - 9); // strip ".envelope" + if (isUuid(stem)) { + return true; + } + // Retry cache filename: "--". + const qsizetype firstSep = stem.indexOf(QLatin1Char('-')); + const qsizetype secondSep = firstSep > 0 ? stem.indexOf(QLatin1Char('-'), firstSep + 1) : -1; + return firstSep > 0 && secondSep > 0 && isUuid(stem.mid(secondSep + 1)) && stem.left(firstSep).toLongLong() > 0; + }; + QStringList result; + const QDir dbDir(databasePath); + if (!dbDir.exists()) { + return result; + } + // Consent-held envelopes (Ask mode: the SDK cached them at crash time instead + // of sending, see sentry__submit_envelope). Bare ".envelope" with + // cache_keep only, "--.envelope" with http_retry. + const QDir cacheDir(databasePath + QStringLiteral("/cache")); + const QStringList cacheNames = cacheDir.entryList({QStringLiteral("*.envelope")}, QDir::Files, QDir::Name); + for (const QString &name : cacheNames) { + if (isEnvelopeName(name)) { + result.append(cacheDir.filePath(name)); + } + } + // In-flight envelopes dumped to the crashed run's own directory (AlwaysSend + // mode: consent was already given, so the transport dump wrote them there and + // sentry_init's process_old_runs replays them at startup — attach before init). + const QStringList runDirs = + dbDir.entryList({QStringLiteral("*.run")}, QDir::Dirs | QDir::NoDotAndDotDot, QDir::Name); + for (const QString &runDir : runDirs) { + const QDir dir(dbDir.filePath(runDir)); + const QStringList names = dir.entryList({QStringLiteral("*.envelope")}, QDir::Files, QDir::Name); + for (const QString &name : names) { + if (isEnvelopeName(name)) { + result.append(dir.filePath(name)); + } + } + } + return result; +} + +int CrashReporter::attachToPendingCrashEnvelopes(const QString &redactedLogText, const QString &userNoteText) +{ + return attachToPendingCrashEnvelopes(sentryDatabasePath, redactedLogText, userNoteText); +} + +int CrashReporter::attachToPendingCrashEnvelopes(const QString &databasePath, + const QString &redactedLogText, + const QString &userNoteText) +{ + if (databasePath.isEmpty()) { + return 0; + } + const QByteArray logBytes = redactedLogText.toUtf8(); + const QByteArray noteBytes = userNoteText.toUtf8(); + int attached = 0; + const QStringList envelopes = pendingEnvelopePaths(databasePath); + for (const QString &path : envelopes) { + bool ok = appendAttachmentToEnvelope(path, QStringLiteral("crash-log.txt"), logBytes); + if (ok && !noteBytes.isEmpty()) { + ok = appendAttachmentToEnvelope(path, QStringLiteral("user-note.txt"), noteBytes); + } + if (ok) { + ++attached; + } else { + qCWarning(CrashReportLog) << "Failed to attach crash log to pending envelope" << path; + } + } + if (attached > 0) { + qCInfo(CrashReportLog) << "Attached crash log and user note to" << attached << "pending crash envelope(s)"; + } + return attached; +} + QString CrashReporter::sanitizeLogLine(const QString &line) { static const auto colorEscapeCodePattern = QRegularExpression(QStringLiteral("\033\\[[0-9;]*m")); diff --git a/cockatrice/src/client/crash/crash_reporter.h b/cockatrice/src/client/crash/crash_reporter.h index f57ec6019..598b6445c 100644 --- a/cockatrice/src/client/crash/crash_reporter.h +++ b/cockatrice/src/client/crash/crash_reporter.h @@ -6,8 +6,10 @@ #ifndef CRASH_REPORTER_H #define CRASH_REPORTER_H +#include #include #include +#include #include #ifndef COCKATRICE_WITH_CRASH_REPORTING @@ -18,6 +20,10 @@ #define COCKATRICE_SENTRY_DSN "" #endif +#ifndef COCKATRICE_SENTRY_RELEASE +#define COCKATRICE_SENTRY_RELEASE "" +#endif + Q_DECLARE_LOGGING_CATEGORY(CrashReportLog); /** @@ -26,12 +32,15 @@ Q_DECLARE_LOGGING_CATEGORY(CrashReportLog); * Single responsibility: init/shutdown the sentry-native SDK behind consent * gating, plus the shared log redaction helpers. An empty DSN (or Never * consent) skips the SDK entirely, so the mock dialog fallback behaves exactly - * as before. init() arms require-user-consent so cached crash envelopes never - * upload before the user is asked; giveConsent()/revokeConsent() (driven by - * the reporting-mode setting and the crash dialog) gate uploads, and any stale - * persisted consent from a previous AlwaysSend run is cleared before init when - * the mode is not AlwaysSend. The real send path is a later increment. All SDK - * calls are guarded so OFF builds compile to no-ops. + * as before. With a DSN, init() arms require-user-consent and before_send + * scrubbing (stale persisted consent from a previous AlwaysSend run is cleared + * before init when the mode is not AlwaysSend); giveConsent()/revokeConsent() + * gate uploads. attachToPendingCrashEnvelopes() folds the redacted log and the + * optional user note into the pending native crash event(s) before consent + * releases them, so dump, stack, log and note arrive as ONE Sentry event; + * sendReport() is the fallback for unclean exits that produced no native crash + * event, capturing a redacted message event with the log preview attached. All + * SDK calls are guarded so OFF builds compile to no-ops. */ class CrashReporter { @@ -49,12 +58,88 @@ public: void shutdown(); void giveConsent(); void revokeConsent(); + bool sendReport(const QString &redactedPreview, const QString &userNote = QString()); + + /** + * @brief Append the redacted log (and optional user note) to every pending + * native crash envelope in the local database, so the crashed event uploads + * with dump, stack, log and note together instead of the log arriving as a + * separate event that has to be correlated by hand. Both storage locations + * are covered: consent-held envelopes in /cache (Ask mode) and + * in-flight envelopes dumped into /.run (AlwaysSend mode). + * + * For the AlwaysSend variant run this BEFORE init() (static overload): the + * startup replay of *.run envelopes happens inside sentry_init. SDK-independent + * plain-file surgery on the Sentry envelope format, so it stays unit-testable + * in OFF builds. + * + * @return the number of envelopes that received the attachments. + */ + int attachToPendingCrashEnvelopes(const QString &redactedLogText, const QString &userNoteText); + static int attachToPendingCrashEnvelopes(const QString &databasePath, + const QString &redactedLogText, + const QString &userNoteText); + + /** + * @brief Append one attachment item to a cached Sentry envelope file. + * + * An envelope is a newline-separated stream of item-header-line + payload + * pairs. The retry transport replays cached envelopes byte-for-byte + * (sentry-native 0.17.3 reads them raw and serializes raw envelopes back + * verbatim), so appending well-formed items associates them with the event + * already in the file; the first line (event id header) is never touched. + * + * @param filename ASCII-only item filename, embedded verbatim in the header. + */ + static bool + appendAttachmentToEnvelope(const QString &envelopePath, const QString &filename, const QByteArray &payload); + + /** + * @brief Pending crash envelopes in : the consent-held files in + * /cache and the in-flight files in /.run. + * + * Matches the cache filename conventions sentry-native accepts (see + * sentry__parse_cache_filename): bare ".envelope" and retry + * "--.envelope"; run dirs accept plain ".envelope" + * minus the SDK's own "__sentry-crash.envelope" marker. + */ + static QStringList pendingEnvelopePaths(const QString &databasePath); /** * @brief SDK-independent gating decision, unit-testable in OFF builds. */ 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/interface/widgets/dialogs/dlg_crash_report.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.cpp index 7fc69e12b..5cd02ebef 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.cpp @@ -15,12 +15,14 @@ #include #include #include +#include #include #include #include #include -DlgCrashReport::DlgCrashReport(QWidget *parent) : QDialog(parent) +DlgCrashReport::DlgCrashReport(QWidget *parent, bool _realSending, CrashReporter *_reporter) + : QDialog(parent), realSending(_realSending), reporter(_reporter) { infoLabel = new QLabel(this); infoLabel->setWordWrap(true); @@ -28,6 +30,13 @@ DlgCrashReport::DlgCrashReport(QWidget *parent) : QDialog(parent) accountLabel = new QLabel(this); accountLabel->setWordWrap(true); + // Constructed before previewArea: Qt derives the default Tab chain from construction + // order, and note sits visually above the log excerpt, so this keeps Tab moving downward. + noteArea = new QPlainTextEdit(this); + // Optional free text ("what I was doing when it crashed") — a few lines are + // enough for a description, leave the rest of the space to the log excerpt. + noteArea->setMaximumHeight(70); + previewArea = new QPlainTextEdit(this); previewArea->setReadOnly(true); previewArea->setPlainText(buildRedactedPreview()); @@ -51,6 +60,7 @@ DlgCrashReport::DlgCrashReport(QWidget *parent) : QDialog(parent) auto *mainLayout = new QVBoxLayout; mainLayout->addWidget(infoLabel); mainLayout->addWidget(accountLabel); + mainLayout->addWidget(noteArea); mainLayout->addWidget(previewArea); mainLayout->addLayout(buttonLayout); setLayout(mainLayout); @@ -70,9 +80,34 @@ void DlgCrashReport::changeEvent(QEvent *event) void DlgCrashReport::retranslateUi() { setWindowTitle(tr("Send crash report")); - infoLabel->setText( - tr("Cockatrice closed unexpectedly. The preview below shows what a crash report would contain (mock).")); - accountLabel->setText(tr("No account needed to send a crash report.")); + if (realSending) { + // The preview below prefers the crashed session's persisted log tail + // (Logger::getPreviousSessionTail), falling back to this restart's in-memory buffer only + // when no previous session was saved. On send it is attached as crash-log.txt to the + // pending native crash event (dump + stack + log + note in one report); when no native + // crash event exists, it rides the fallback message event instead. + infoLabel->setText(tr("Oops, we crashed — and now we need your help to make Cockatrice better!\n\n" + "The good thing is, helping is easy and convenient. Sending uploads what the " + "previous run left behind when it crashed: the crash dump with the stack trace, " + "the app version and operating system info, the redacted log excerpt shown below, " + "and anything you write in the note box. If no log from the crashed session was " + "saved, this restart's early logs are shown instead.\n\n" + "Reports are what let us fix crashes like this one. Check \u201cRemember my choice\u201d " + "and Send, and future crash reports are sent automatically without interruption.")); + } else { + infoLabel->setText(tr("(Mock preview — nothing will be uploaded.)\n\n" + "Cockatrice closed unexpectedly. A real report would contain what the previous run " + "left behind when it crashed: the crash dump with the stack trace, the app version " + "and operating system info, the redacted log excerpt shown below, and anything you " + "write in the note box. If no log from the crashed session was saved, this restart's " + "early logs are shown instead.\n\n" + "A mock file is written locally for inspection.")); + } + accountLabel->setText(tr("No account needed, and no contact info is collected. Login links " + "(cockatrice://...) are scrubbed before sending.")); + noteArea->setPlaceholderText(tr("(Optional) Describe what you were doing when it crashed")); + // Direction-neutral on purpose: the same box also remembers a "Don't send" decision + // (reject() persists NeverSend when it is checked), so it must not promise sending. rememberBox->setText(tr("Remember my choice")); sendButton->setText(tr("Send")); dontSendButton->setText(tr("Don't send")); @@ -103,7 +138,7 @@ QString DlgCrashReport::buildRedactedPreview() return lines.join(QStringLiteral("\n")); } -QString DlgCrashReport::writeMockReport(const QString &previewText) +QString DlgCrashReport::writeMockReport(const QString &previewText, const QString &userNote) { Logger &logger = Logger::getInstance(); @@ -115,6 +150,7 @@ QString DlgCrashReport::writeMockReport(const QString &previewText) report[QStringLiteral("systemLocale")] = logger.getSystemLocale(); report[QStringLiteral("installInfo")] = logger.getClientInstallInfo(); report[QStringLiteral("logPreview")] = previewText; + report[QStringLiteral("userNote")] = userNote; QString fileName = QStringLiteral("crash-report-mock-%1.json") .arg(QDateTime::currentDateTimeUtc().toString(QStringLiteral("yyyyMMdd-HHmmss"))); @@ -134,7 +170,56 @@ void DlgCrashReport::actSend() if (rememberBox->isChecked()) { SettingsCache::instance().personal().setCrashReportingMode(PersonalSettings::CrashReportingMode::AlwaysSend); } - const QString savedPath = writeMockReport(previewArea->toPlainText()); + const QString preview = previewArea->toPlainText(); + const QString userNote = noteArea->toPlainText().trimmed(); + if (realSending && reporter != nullptr) { + // Fold log and note into the pending native crash event(s) BEFORE consent: + // giveConsent() releases the held envelopes for upload, so dump, stack, + // log and note arrive as one Sentry event. Without a pending crash event + // (unclean exit that produced no native crash) this returns 0 and + // sendReport() falls back to a standalone message event with the same + // attachments. + const int attached = reporter->attachToPendingCrashEnvelopes(preview, userNote); + reporter->giveConsent(); + if (attached > 0 || reporter->sendReport(preview, userNote)) { + accept(); + return; + } + qCWarning(CrashReportLog) << "Failed to send crash report"; + // Outcome mapping for the failure box: Save = store locally and close + // as accepted (the follow-up info box shows the path, so the user + // knows it was NOT sent); Discard = revoke consent and close as + // rejected (dropped); Cancel/X = back to the dialog, neither sent nor + // dropped. The Remember checkbox governs only the original + // Send/Don't-send decision, so the Discard path below revokes and + // closes via QDialog::reject() without touching the setting (calling + // our reject() override here would wrongly persist NeverSend over the + // AlwaysSend stored above when Remember was checked). + QMessageBox failureBox(QMessageBox::Warning, tr("Send crash report"), + tr("Sending the crash report failed. Save the report locally instead?"), + QMessageBox::Save | QMessageBox::Discard | QMessageBox::Cancel, this); + failureBox.setDefaultButton(QMessageBox::Save); + failureBox.setEscapeButton(QMessageBox::Cancel); + const QMessageBox::StandardButton choice = static_cast(failureBox.exec()); + if (choice == QMessageBox::Save) { + const QString savedPath = writeMockReport(preview, userNote); + if (savedPath.isEmpty()) { + QMessageBox::warning(this, tr("Send crash report"), tr("Could not save the crash report locally.")); + } else { + QMessageBox::information(this, tr("Send crash report"), + tr("Crash report saved locally to:\n%1").arg(savedPath)); + } + accept(); + return; + } + if (choice == QMessageBox::Discard) { + reporter->revokeConsent(); + QDialog::reject(); + return; + } + return; + } + const QString savedPath = writeMockReport(preview, userNote); if (savedPath.isEmpty()) { qCWarning(CrashReportLog) << "Could not save the mock crash report locally"; } else { @@ -144,9 +229,17 @@ void DlgCrashReport::actSend() } void DlgCrashReport::actDontSend() +{ + reject(); +} + +void DlgCrashReport::reject() { if (rememberBox->isChecked()) { SettingsCache::instance().personal().setCrashReportingMode(PersonalSettings::CrashReportingMode::NeverSend); } - reject(); + if (realSending && reporter != nullptr) { + reporter->revokeConsent(); + } + QDialog::reject(); } diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.h b/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.h index 576eb3cf9..83808fcbd 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.h +++ b/cockatrice/src/interface/widgets/dialogs/dlg_crash_report.h @@ -15,20 +15,29 @@ class QLabel; class QPlainTextEdit; class QPushButton; +class CrashReporter; + /** - * @brief MOCK crash-report dialog (UI + settings only, no network). + * @brief Crash-report dialog: mock fallback or real Sentry send. * - * The Send path writes a redacted JSON file to the temp dir instead of - * uploading anything. Real Sentry upload comes in a later phase. + * Mock mode (default): the Send path writes a redacted JSON file to the temp + * dir instead of uploading anything. Real mode (realSending, with a live + * CrashReporter): Send first folds the redacted log and the optional user note + * into the pending native crash event(s), then gives consent so dump, stack, + * log and note upload as one Sentry event; when no pending crash event exists + * it falls back to a standalone message event with the same attachments. + * Failure offers a local save instead. */ class DlgCrashReport : public QDialog { Q_OBJECT public: - explicit DlgCrashReport(QWidget *parent = nullptr); + explicit DlgCrashReport(QWidget *parent = nullptr, bool realSending = false, CrashReporter *_reporter = nullptr); static QString buildRedactedPreview(); - static QString writeMockReport(const QString &previewText); + static QString writeMockReport(const QString &previewText, const QString &userNote = QString()); + + void reject() override; protected: void changeEvent(QEvent *event) override; @@ -37,9 +46,12 @@ private: QLabel *infoLabel; QLabel *accountLabel; QPlainTextEdit *previewArea; + QPlainTextEdit *noteArea; QCheckBox *rememberBox; QPushButton *sendButton; QPushButton *dontSendButton; + bool realSending = false; + CrashReporter *reporter = nullptr; void retranslateUi(); diff --git a/cockatrice/src/main.cpp b/cockatrice/src/main.cpp index 38ec3d137..fd98b1945 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -264,23 +264,66 @@ int main(int argc, char *argv[]) qApp->setAttribute(Qt::AA_DontShowIconsInMenus, true); #endif - // Crash reporting (increment 1): init the SDK early so a crash anywhere below is captured into the + // Persistent session log: keeps this run's logs on disk so the next-startup crash dialog can + // attach the crashed session's tail (the in-memory buffer is lost on crash). Opened in Append + // mode with a per-launch session marker; read back via Logger::getPreviousSessionTail(). + // Declared before crash-reporter init: the startup attach below needs the log tail while the + // file still holds the crashed session's lines. + const QString persistentLogPath = + SettingsCache::instance().getSettingsPath() + QStringLiteral("logs/cockatrice.log"); + Logger::getInstance().setPersistentLogPath(persistentLogPath); + + // Crash reporting: init the SDK early so a crash anywhere below is captured into the // local database. Uploads stay consent-gated (require_user_consent, armed in CrashReporter::init): // AlwaysSend gives consent here so cached envelopes flush at startup; any other mode starts // unconsented and the dialog below drives give/revoke. An empty DSN skips the SDK entirely and // the mock fallback further below behaves exactly as before. CrashReporter crashReporter; const QString crashDatabasePath = SettingsCache::instance().getSettingsPath() + QStringLiteral("sentry"); + // Envelopes that received the redacted log at startup (AlwaysSend path); 0 means the log was + // NOT folded into a pending crash event and a standalone log event is still needed below. + int crashLogAttachedEnvelopes = 0; { QString crashDsn = QString::fromUtf8(qgetenv("COCKATRICE_SENTRY_DSN")); - if (crashDsn.isEmpty()) { - crashDsn = QString::fromLatin1(COCKATRICE_SENTRY_DSN); + const bool dsnFromEnvironment = !crashDsn.isEmpty(); + if (!dsnFromEnvironment) { + crashDsn = QString::fromUtf8(COCKATRICE_SENTRY_DSN); } - crashReporter.init(crashDsn, QString::fromLatin1(VERSION_STRING), crashDatabasePath, - SettingsCache::instance().personal().getCrashReportingMode()); - if (crashReporter.isReady() && SettingsCache::instance().personal().getCrashReportingMode() == - PersonalSettings::CrashReportingMode::AlwaysSend) { - // Silent opt-in: cached crash envelopes flush automatically once consent is given. + 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 + // envelope(s) BEFORE init: sentry_init replays the crashed run's *.run + // envelopes immediately when consent is already persisted, so attaching any + // later would be too late. Consent given below then releases them, and dump, + // stack and log upload as a single Sentry event. Ask mode attaches at the + // dialog instead — its cache-held envelopes stay untouched until giveConsent(). + if (crashMode == PersonalSettings::CrashReportingMode::AlwaysSend && + CrashReporter::isReportingEnabled(crashDsn, crashMode)) { + crashLogAttachedEnvelopes = CrashReporter::attachToPendingCrashEnvelopes( + crashDatabasePath, DlgCrashReport::buildRedactedPreview(), QString()); + } + crashReporter.init(crashDsn, QString::fromUtf8(COCKATRICE_SENTRY_RELEASE), crashDatabasePath, crashMode); + if (crashReporter.isReady() && crashMode == PersonalSettings::CrashReportingMode::AlwaysSend) { + // Silent opt-in: cached native crash envelopes flush automatically once consent is given. crashReporter.giveConsent(); } // Mid-session mode changes must reach the SDK: without this, switching from AlwaysSend to @@ -297,13 +340,6 @@ int main(int argc, char *argv[]) }); } - // Persistent session log: keeps this run's logs on disk so the next-startup crash dialog can - // attach the crashed session's tail (the in-memory buffer is lost on crash). Opened in Append - // mode with a per-launch session marker; read back via Logger::getPreviousSessionTail(). - const QString persistentLogPath = - SettingsCache::instance().getSettingsPath() + QStringLiteral("logs/cockatrice.log"); - Logger::getInstance().setPersistentLogPath(persistentLogPath); - // Translations #ifdef Q_OS_MAC translationPath = qApp->applicationDirPath() + "/../Resources/translations"; @@ -323,7 +359,8 @@ int main(int argc, char *argv[]) {{{"c", "connect"}, QCoreApplication::translate("main", "Connect on startup"), "user:pass@host:port"}, {{"d", "debug-output"}, QCoreApplication::translate("main", "Debug to file")}, {"simulate-crash", - QCoreApplication::translate("main", "Show the mock crash-report dialog (nothing is sent)")}}); + QCoreApplication::translate("main", + "Show the crash-report dialog (saves a mock file when no DSN is configured)")}}); parser.process(app); @@ -460,10 +497,11 @@ int main(int argc, char *argv[]) ui.show(); qCInfo(MainLog) << "ui.show() finished"; - // MOCK crash-report flow (UI + settings only): no sentry-native upload, no network. - // Triggered by --simulate-crash or by leftover Windows crash dumps. Handled dumps are deleted - // after Send / Don't send (or a silent NeverSend skip) so one crash never prompts twice; on - // non-Windows nothing writes cockatrice.crash.*.dmp yet, so --simulate-crash is the trigger there. + // Crash-report flow: with a configured DSN the SDK owns capture and upload + // (consent-gated); with an empty DSN the dialog falls back to a local mock + // file and behaves exactly as before. Triggered by --simulate-crash or by + // leftover Windows crash dumps, which are consumed after handling so one + // crash never prompts twice. { QString crashDumpDir; #ifdef Q_OS_WIN @@ -487,18 +525,31 @@ int main(int argc, char *argv[]) if (parser.isSet("simulate-crash") || !crashDumps.isEmpty()) { const PersonalSettings::CrashReportingMode crashMode = SettingsCache::instance().personal().getCrashReportingMode(); + const bool crashReady = crashReporter.isReady(); if (crashMode == PersonalSettings::CrashReportingMode::AlwaysSend) { - crashReporter.giveConsent(); - DlgCrashReport::writeMockReport(DlgCrashReport::buildRedactedPreview()); - } else if (crashMode == PersonalSettings::CrashReportingMode::Ask) { - DlgCrashReport crashDialog(&ui); - if (crashDialog.exec() == QDialog::Accepted) { - crashReporter.giveConsent(); + if (crashReady) { + // Silent path: the redacted log was folded into the pending native crash + // envelope(s) at startup and consent already released them. Only when no + // pending crash event existed (e.g. --simulate-crash) does a standalone + // log event still need to go out here. + if (crashLogAttachedEnvelopes == 0) { + const QString crashPreview = DlgCrashReport::buildRedactedPreview(); + if (!crashReporter.sendReport(crashPreview)) { + const QString savedPath = DlgCrashReport::writeMockReport(crashPreview); + qCWarning(MainLog) << "Failed to send crash report, saved locally to" << savedPath; + } + } } else { - crashReporter.revokeConsent(); + DlgCrashReport::writeMockReport(DlgCrashReport::buildRedactedPreview()); } + } else if (crashMode == PersonalSettings::CrashReportingMode::Ask) { + // Real mode: Send gives consent + captures the event, Don't + // send revokes consent; the dialog owns both via the reporter. + DlgCrashReport crashDialog(&ui, crashReady, crashReady ? &crashReporter : nullptr); + crashDialog.exec(); } // NeverSend: skip silently (no dialog, nothing written or sent). + // Handled dumps are consumed so one crash never prompts twice. for (const QString &dump : crashDumps) { QFile::remove(crashDumpDir + QStringLiteral("/") + dump); } diff --git a/tests/crash_reporter_test.cpp b/tests/crash_reporter_test.cpp index c52a2e495..e15007f7a 100644 --- a/tests/crash_reporter_test.cpp +++ b/tests/crash_reporter_test.cpp @@ -1,6 +1,10 @@ #include "client/crash/crash_reporter.h" +#include +#include +#include #include +#include #include #include #include @@ -39,7 +43,7 @@ TEST(CrashReporterTest, RedactCockatriceUrlsLeavesOtherTextAlone) TEST(CrashReporterTest, InitSkippedWithoutDsn) { CrashReporter reporter; - reporter.init(QString(), QStringLiteral("3.1.2"), QStringLiteral("/tmp/cockatrice-crash-test"), + reporter.init(QString(), QStringLiteral("3.1.2"), QDir::tempPath() + QStringLiteral("/cockatrice-crash-test"), PersonalSettings::CrashReportingMode::Ask); ASSERT_FALSE(reporter.isReady()); reporter.shutdown(); @@ -50,7 +54,8 @@ TEST(CrashReporterTest, InitSkippedWhenNeverSend) { CrashReporter reporter; reporter.init(QStringLiteral("https://key@sentry.io/1"), QStringLiteral("3.1.2"), - QStringLiteral("/tmp/cockatrice-crash-test"), PersonalSettings::CrashReportingMode::NeverSend); + QDir::tempPath() + QStringLiteral("/cockatrice-crash-test"), + PersonalSettings::CrashReportingMode::NeverSend); ASSERT_FALSE(reporter.isReady()); reporter.shutdown(); } @@ -70,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; @@ -78,6 +145,200 @@ TEST(CrashReporterTest, ShutdownWithoutInitIsSafe) ASSERT_FALSE(reporter.isReady()); } +TEST(CrashReporterTest, ConsentAndSendWithoutDsnAreSafeNoOps) +{ + CrashReporter reporter; + reporter.init(QString(), QStringLiteral("3.1.2"), QDir::tempPath() + QStringLiteral("/cockatrice-crash-test"), + PersonalSettings::CrashReportingMode::Ask); + ASSERT_FALSE(reporter.isReady()); + reporter.giveConsent(); + reporter.revokeConsent(); + ASSERT_FALSE(reporter.isReady()); + ASSERT_FALSE(reporter.sendReport(QStringLiteral("preview"))); + ASSERT_FALSE(reporter.sendReport(QString())); + reporter.shutdown(); + ASSERT_FALSE(reporter.isReady()); +} + +namespace +{ + +QByteArray readFileBytes(const QString &path) +{ + QFile file(path); + if (!file.open(QIODevice::ReadOnly)) { + return {}; + } + return file.readAll(); +} + +bool writeFileBytes(const QString &path, const QByteArray &content) +{ + QFile file(path); + if (!file.open(QIODevice::WriteOnly | QIODevice::Truncate)) { + return false; + } + return file.write(content) == content.size(); +} + +QString uuidOf(char marker) +{ + // 36 chars, 4 hyphens — structurally a UUID, content irrelevant for these tests. + return QStringLiteral("00000000-0000-4000-8000-00000000000%1").arg(marker); +} + +} // namespace + +TEST(CrashReporterTest, AppendAttachmentAppendsWellFormedItemAfterExistingContent) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString path = dir.filePath(QStringLiteral("event.envelope")); + // Existing envelope content deliberately WITHOUT a trailing newline. + const QByteArray original = QByteArrayLiteral("{\"event_id\":\"x\"}\n{\"type\":\"event\",\"length\":2}\n{}"); + ASSERT_TRUE(writeFileBytes(path, original)); + + ASSERT_TRUE( + CrashReporter::appendAttachmentToEnvelope(path, QStringLiteral("crash-log.txt"), QByteArrayLiteral("hello"))); + const QByteArray expected = original + '\n' + + QByteArrayLiteral("{\"type\":\"attachment\",\"length\":5,\"attachment_length\":5," + "\"filename\":\"crash-log.txt\",\"content_type\":\"text/plain\"}\n") + + QByteArrayLiteral("hello"); + ASSERT_EQ(readFileBytes(path), expected); +} + +TEST(CrashReporterTest, AppendAttachmentOnEmptyFileHasNoLeadingNewline) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString path = dir.filePath(QStringLiteral("event.envelope")); + ASSERT_TRUE(writeFileBytes(path, QByteArray())); + + ASSERT_TRUE( + CrashReporter::appendAttachmentToEnvelope(path, QStringLiteral("user-note.txt"), QByteArrayLiteral("note"))); + const QByteArray expected = QByteArrayLiteral("{\"type\":\"attachment\",\"length\":4,\"attachment_length\":4," + "\"filename\":\"user-note.txt\",\"content_type\":\"text/plain\"}\n") + + QByteArrayLiteral("note"); + ASSERT_EQ(readFileBytes(path), expected); +} + +TEST(CrashReporterTest, AppendAttachmentRejectsMissingFile) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + ASSERT_FALSE(CrashReporter::appendAttachmentToEnvelope(dir.filePath(QStringLiteral("missing.envelope")), + QStringLiteral("crash-log.txt"), QByteArrayLiteral("x"))); +} + +TEST(CrashReporterTest, AppendAttachmentLengthIsByteAccurate) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString path = dir.filePath(QStringLiteral("event.envelope")); + ASSERT_TRUE(writeFileBytes(path, QByteArrayLiteral("{}"))); + + const QByteArray utf8Payload = QString::fromUtf8("héllo").toUtf8(); + ASSERT_EQ(utf8Payload.size(), 6); // 5 characters, "é" is 2 bytes + ASSERT_TRUE(CrashReporter::appendAttachmentToEnvelope(path, QStringLiteral("crash-log.txt"), utf8Payload)); + const QByteArray content = readFileBytes(path); + ASSERT_TRUE(content.contains(QByteArrayLiteral("\"length\":6"))); + ASSERT_TRUE(content.contains(QByteArrayLiteral("\"attachment_length\":6"))); + ASSERT_TRUE(content.endsWith(utf8Payload)); +} + +// The SDK's envelope re-parser frames each item's payload by its `length` header; a +// missing or wrong length ends the payload at the first newline, so the remaining log +// lines get parsed as item headers and the whole envelope is rejected (and deleted). +TEST(CrashReporterTest, AppendAttachmentKeepsMultiLinePayloadIntact) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString path = dir.filePath(QStringLiteral("event.envelope")); + ASSERT_TRUE(writeFileBytes(path, QByteArrayLiteral("{}"))); + + const QByteArray payload = QByteArrayLiteral("first line\nsecond line\nthird line"); + ASSERT_TRUE(CrashReporter::appendAttachmentToEnvelope(path, QStringLiteral("crash-log.txt"), payload)); + const QByteArray content = readFileBytes(path); + ASSERT_TRUE(content.contains("\"length\":" + QByteArray::number(payload.size()))); + ASSERT_TRUE(content.contains("\"attachment_length\":" + QByteArray::number(payload.size()))); + ASSERT_TRUE(content.endsWith(payload)); +} + +TEST(CrashReporterTest, PendingEnvelopePathsMatchesSdkNamingInCacheAndRunDirs) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString dbPath = dir.path(); + ASSERT_TRUE(QDir().mkpath(dbPath + QStringLiteral("/cache"))); + ASSERT_TRUE(QDir().mkpath(dbPath + QStringLiteral("/aaaa.run"))); + ASSERT_TRUE(QDir().mkpath(dbPath + QStringLiteral("/other"))); + + const QString retryName = QStringLiteral("1700000000000-0-") + uuidOf('a') + QStringLiteral(".envelope"); + const QString bareName = uuidOf('b') + QStringLiteral(".envelope"); + // Valid: retry-style cache file + bare run-dir file. + ASSERT_TRUE(writeFileBytes(dbPath + QStringLiteral("/cache/") + retryName, QByteArrayLiteral("{}"))); + ASSERT_TRUE(writeFileBytes(dbPath + QStringLiteral("/aaaa.run/") + bareName, QByteArrayLiteral("{}"))); + // Rejected: junk names and the SDK's own marker file. + ASSERT_TRUE(writeFileBytes(dbPath + QStringLiteral("/cache/") + uuidOf('c') + QStringLiteral(".envelope.bak"), + QByteArrayLiteral("{}"))); + ASSERT_TRUE(writeFileBytes(dbPath + QStringLiteral("/cache/") + QStringLiteral("random.envelope"), + QByteArrayLiteral("{}"))); + ASSERT_TRUE(writeFileBytes(dbPath + QStringLiteral("/aaaa.run/") + QStringLiteral("__sentry-crash.envelope"), + QByteArrayLiteral("{}"))); + ASSERT_TRUE(writeFileBytes(dbPath + QStringLiteral("/other/") + bareName, QByteArrayLiteral("{}"))); + + const QStringList found = CrashReporter::pendingEnvelopePaths(dbPath); + ASSERT_EQ(found.size(), 2); + ASSERT_TRUE(found.contains(dbPath + QStringLiteral("/cache/") + retryName)); + ASSERT_TRUE(found.contains(dbPath + QStringLiteral("/aaaa.run/") + bareName)); +} + +TEST(CrashReporterTest, PendingEnvelopePathsWithoutDatabaseIsEmpty) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + ASSERT_TRUE(CrashReporter::pendingEnvelopePaths(dir.filePath(QStringLiteral("no-such-db"))).isEmpty()); + ASSERT_TRUE(CrashReporter::pendingEnvelopePaths(QString()).isEmpty()); +} + +TEST(CrashReporterTest, AttachFoldsLogAndNoteIntoPendingEnvelopes) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString dbPath = dir.path(); + ASSERT_TRUE(QDir().mkpath(dbPath + QStringLiteral("/cache"))); + const QString envelopePath = dbPath + QStringLiteral("/cache/") + uuidOf('d') + QStringLiteral(".envelope"); + ASSERT_TRUE(writeFileBytes(envelopePath, QByteArrayLiteral("{\"event_id\":\"x\"}\n{}\n"))); + + CrashReporter reporter; + // Empty DSN: no SDK, but init() still records the database path the dialog uses. + reporter.init(QString(), QStringLiteral("3.1.2"), dbPath, PersonalSettings::CrashReportingMode::Ask); + ASSERT_EQ(reporter.attachToPendingCrashEnvelopes(QStringLiteral("log line"), QStringLiteral("my note")), 1); + + const QByteArray content = readFileBytes(envelopePath); + ASSERT_TRUE(content.startsWith(QByteArrayLiteral("{\"event_id\":\"x\"}\n{}\n"))); + ASSERT_TRUE(content.contains(QByteArrayLiteral("\"filename\":\"crash-log.txt\""))); + ASSERT_TRUE(content.contains(QByteArrayLiteral("\nlog line"))); + ASSERT_TRUE(content.contains(QByteArrayLiteral("\"filename\":\"user-note.txt\""))); + ASSERT_TRUE(content.endsWith(QByteArrayLiteral("\nmy note"))); +} + +TEST(CrashReporterTest, AttachSkipsNoteWhenEmptyAndFindsRunDirEnvelopes) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString dbPath = dir.path(); + ASSERT_TRUE(QDir().mkpath(dbPath + QStringLiteral("/bbbb.run"))); + const QString envelopePath = dbPath + QStringLiteral("/bbbb.run/") + uuidOf('e') + QStringLiteral(".envelope"); + ASSERT_TRUE(writeFileBytes(envelopePath, QByteArrayLiteral("{}"))); + + // Static overload (the pre-init AlwaysSend path) with an empty note. + ASSERT_EQ(CrashReporter::attachToPendingCrashEnvelopes(dbPath, QStringLiteral("log only"), QString()), 1); + const QByteArray content = readFileBytes(envelopePath); + ASSERT_TRUE(content.contains(QByteArrayLiteral("\"filename\":\"crash-log.txt\""))); + ASSERT_FALSE(content.contains(QByteArrayLiteral("user-note.txt"))); +} + } // namespace int main(int argc, char **argv) From 9f59f5c495f80432d9fd70c224fa88f86f30e6ab Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 19 Oct 2026 14:57:04 +0200 Subject: [PATCH 03/90] [CrashReporting] Breakpad capture, session detection and symbol upload (#7523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [CrashReporting] Breakpad everywhere, session detection, symbol CI (inc 3) Breakpad on all platforms (no crashpad packaging); remove the Windows MiniDumpWithFullMemory filter and the installer WER LocalDumps full-dump registration. QLockFile per-process session tracking replaces the Windows-only .dmp scan, placed after parser.process and the instance hand-off so help/version exits and handed-off secondaries never leak a lock. CI: Release-only DSN bake from SENTRY_DSN secret, tag-only sentry-cli symbol upload (org cockatrice, project native). Tests: 533/533 ctest green, format clean, code gate PASS. * [CrashReporting] Enable http_retry, harden symbol upload, fix NSIS context Sets sentry_options_set_http_retry so the cache_keep-held crash envelope in Ask mode is written with a retry filename and actually flushes on giveConsent() instead of sitting in /cache until pruning. Adds continue-on-error to both sentry-cli install steps so a Sentry registry outage or broken Git Bash install cannot block release assets. Cleans the legacy WER CrashDumps folder under SetShellVarContext current (under all it resolves to ProgramData, not the user's AppData) in installer and uninstaller; registry delete works in either context. * [CrashReporting] Extract the startup crash-report flow from main() per review * [CrashReporting] Break up the CrashReporter class doc per review * [CrashReporting] Drop the invalid --release from the symbol upload `debug-files upload` never accepted `--release` (checked against the pinned sentry-cli 3.8.0: the subcommand declares --org/--project and a set of type/id/scan options, and execute() reads no release; the only global flags are --url/--header/--auth-token/--log-level/--quiet/--allow-failure). Clap rejects the unknown flag before any upload happens, so every tag build has been shipping without symbols, with continue-on-error keeping the release pipeline green. Release association is also the wrong mechanism: debug information files are matched to events by debug ID via the event's debug_meta. The Sentry release of a native event comes from the event itself (COCKATRICE_SENTRY_RELEASE), not from the DIF upload. Remove the flag and the now-unused SENTRY_RELEASE env from both invocations (Linux, and Windows/macOS). The symbol upload stays continue-on-error (a Sentry outage must not block the release assets); revisit once it has proven itself on a tag. * [CrashReporting] Split debug information out of packaged release binaries -g is required so the CI symbol upload has DWARF to send, but it was left in the shipped binaries too: the Linux Release client is 563 MiB, 552 MiB of it debug info, and every distribution channel pays for that. Keep the build tree unchanged (CI uploads symbols from build/) and strip only the packaged copy. A CPack pre-build script extracts each ELF's debug info to COCKATRICE_DEBUG_SYMBOL_DIR and then runs `strip --strip-debug --strip-unneeded`. .eh_frame is allocatable and survives, so unwinding is unaffected, and the build ID is identical in the stripped binary and the sidecar. Measured on a local Release build (GCC -O2 -g, WITH_CRASH_REPORTING=ON), bytes: binary build tree packaged before packaged after .debug sidecar cockatrice 590,722,560 590,722,560 28,086,480 579,623,576 oracle 53,432,608 53,432,608 1,649,888 52,075,344 tar.gz of both trees: 289,964,521 -> 19,018,058 (~15x smaller). Only ELF (Linux/BSD) is handled: Windows already links /DEBUG with separate PDBs, and macOS extracts a dSYM from the bundle. Opt-out via COCKATRICE_SPLIT_DEBUG_SYMBOLS; skipped with a warning when objcopy/strip are missing. CPACK_PRE_BUILD_SCRIPTS (not CPACK_INSTALL_SCRIPTS, which runs before staging) invokes it, and the script refuses to run unless CPACK_TEMPORARY_DIRECTORY is a real CPack staging tree so a missing variable can never turn its glob into a filesystem-wide walk. * [CrashReporting] Upload Windows PDBs from where Ninja actually puts them The Windows branch of the symbol upload looked in build//Release, which is the Visual Studio generator's layout. The Windows leg uses Ninja, which is single-config: linker PDBs land in build//, next to the binaries — the same paths the "[Windows] Upload PDBs" artifact step below globs with if-no-files-found: error. So paths was always empty on Windows, the step printed "No symbol paths found, skipping symbol upload" and exited 0: a silent no-op behind continue-on-error. Every Windows build would then report "A required debug information file was missing" for cockatrice.exe with a PDB-style debug id (GUID-age), exactly as if no upload step existed at all. Use the artifact step's globs. Also turn the empty-path case into a failure in both symbol steps: a silent skip is indistinguishable from a successful upload (which is how the wrong path went unnoticed), and continue-on-error already keeps a broken upload from failing the release. Verified by extracting both steps' run blocks and executing the Windows branch against a Ninja-style tree: it now passes the three linker PDBs to sentry-cli (and not the CMakeFiles/*.pdb intermediates), and exits non-zero with an ::error:: annotation when there is nothing to upload. * [CrashReporting] Archive PDBs on tag builds so a failed upload is recoverable The "[Windows] Upload PDBs" artifact step skipped tag builds (github.ref_type != 'tag'), so for a release the symbols existed only inside the Sentry upload — which is continue-on-error. A failed upload therefore lost the ability to symbolicate that release permanently: no rebuild produces the same PDB, since the debug id is a per-build GUID+age. That is how every Windows crash reported so far became unreadable. Archive the PDBs for tag builds as well, and order the step after "Upload to release" so a missing PDB can never block a release from being published. Retention stays at the repository default; nothing consumes the artifact automatically, it is the manual recovery path (gh run download -n -PDBs, then sentry-cli debug-files upload). Verified by re-parsing the workflow and asserting the step order (release -> PDBs -> attest) and the broadened condition. * [CrashReporting] Upload the Windows executables with their PDBs Sentry needs two things per image on Windows: symbolication and stack unwinding. The PDB covers symbolication, but for a 64-bit PE the Call Frame Information (.pdata/.xdata) lives in the executable and is not copied into the PDB, so uploading PDBs alone leaves the image at "Stack Unwinding: Missing". That is exactly what the event's Images panel showed for cockatrice.exe, while Sentry's own Microsoft symbol server resolved KERNELBASE.dll and ucrtbase.dll from the same dump. sentry-cli also needs the pair in the same invocation: only then does it associate the PE's code identifier with the PDB's debug identifier, otherwise "the identifiers diverge and Sentry might not be able to resolve the correct file for symbolication" (docs: Debug Identifiers). Do the same for macOS, where the dSYM carries the debug information but the Mach-O carries the unwind information, and include the executables in the symbol artifact so a re-upload still has the pair to offer. Verified by extracting the step and running both branches against stub harnesses: Windows passes the three exe/pdb pairs in a single sentry-cli invocation and still fails loudly on an empty tree; macOS passes the dSYM and the Mach-O together. * [CrashReporting] Upload the shipped Qt runtime DLLs too Qt ships no PDBs with its MSVC SDK, so every Qt frame in a crash report resolved to nothing: an image with neither PDB nor PE has no unwind information (on 64-bit Windows the Call Frame Information lives in the PE and is not copied into the PDB) and no symbol source to fall back on. The list is required-runtime.txt, the manifest .ci/compile.sh already asserts the installer against, so what gets uploaded is exactly what an install carries. Qt frames now get unwinding plus export-table names; line numbers still need Qt's debug information files, which the SDK omits. The guards are not cosmetic: the step runs under `bash -eo pipefail`, where a sed over a missing file or an unmatched ls glob aborts the whole step, and continue-on-error makes that look identical to a successful upload. * [CrashReporting] Upload only the binaries, archive symbols only on tags The Linux leg handed sentry-cli whole build directories: 471 candidate files, of which 7 were object files that can never match a stack frame. Only the three linked binaries carry the build-ids Sentry looks up; the packaged copy is split and stripped but keeps that same build-id. The Windows symbol archive ran on every packaging build even though only tag builds upload to Sentry, so a PR build paid for an artifact nothing could ever consume. Tags keep it as the durable copy for a failed upload. --------- Co-authored-by: Lukas Brübach --- .ci/compile.sh | 8 + .github/workflows/desktop-build.yml | 222 +++++++++++++++++- cmake/CrashReporting.cmake | 60 ++++- cmake/NSIS.template.in | 28 ++- cmake/SplitDebugSymbols.cmake.in | 80 +++++++ .../src/client/crash/crash_reporter.cpp | 90 ++++++- cockatrice/src/client/crash/crash_reporter.h | 54 +++-- cockatrice/src/main.cpp | 183 +++++---------- tests/crash_reporter_test.cpp | 79 +++++++ 9 files changed, 648 insertions(+), 156 deletions(-) create mode 100644 cmake/SplitDebugSymbols.cmake.in diff --git a/.ci/compile.sh b/.ci/compile.sh index 2aeeaa6ec..63c027473 100755 --- a/.ci/compile.sh +++ b/.ci/compile.sh @@ -206,6 +206,14 @@ if [[ ${WITH_CRASH_REPORTING:-} == "ON" ]]; then # developer builds never reach Sentry. flags+=("-DWITH_CRASH_REPORTING=ON") fi +if [[ ${BUILDTYPE:-Release} == "Release" && -n ${COCKATRICE_SENTRY_DSN:-} ]]; then + # Release-only: Debug builds stay DSN-free so crashes from developer builds + # never reach Sentry. Empty (local builds, forks without the secret, and non-tag + # CI builds, which never receive the secret) keeps today's mock fallback via the + # compiled-in default. CI passes the value from the SENTRY_DSN secret on tag + # Release builds only, so pre-release test artifacts never report to Sentry. + flags+=("-DCOCKATRICE_SENTRY_DSN=$COCKATRICE_SENTRY_DSN") +fi # Add cmake --build flags buildflags=(--config "$BUILDTYPE") diff --git a/.github/workflows/desktop-build.yml b/.github/workflows/desktop-build.yml index 2215ac2d2..c97b48342 100644 --- a/.github/workflows/desktop-build.yml +++ b/.github/workflows/desktop-build.yml @@ -46,6 +46,11 @@ jobs: outputs: tag: ${{ steps.configure.outputs.tag }} sha: ${{ steps.configure.outputs.sha }} + # MAJ.MIN.PATCH[-beta[.N]] of a tag build; empty otherwise. Mirrors what + # cmake/getversion.cmake derives as PROJECT_VERSION, which the client bakes as its + # Sentry release (cmake/CrashReporting.cmake) — symbol uploads must use the same + # string so stack traces de-symbolicate against this build's binaries. + sentry_release: ${{ steps.configure.outputs.sentry_release }} steps: - name: "Configure" @@ -56,6 +61,9 @@ jobs: run: | if [[ "$GITHUB_REF_TYPE" == 'tag' ]]; then # release echo "tag=$GITHUB_REF_NAME" >> "$GITHUB_OUTPUT" + # Tag format is YYYY-MM-DD-{Release|Development}-MAJ.MIN.PATCH[-beta[.N]]; + # PROJECT_VERSION is the tail after the type (see cmake/getversion.cmake). + echo "sentry_release=$(sed -E 's/^[0-9]{4}-[0-9]{2}-[0-9]{2}-(Release|Development)-//' <<< "$GITHUB_REF_NAME")" >> "$GITHUB_OUTPUT" fi echo "sha=$RESOLVED_SHA" >> "$GITHUB_OUTPUT" @@ -240,6 +248,11 @@ jobs: package: '${{ matrix.package }}' server_only: '${{ matrix.server_only }}' WITH_CRASH_REPORTING: 'ON' + # Tag builds only: artifacts from master pushes and PR branches must not report to + # Sentry. Forks get no secrets on pull_request, so they keep the mock fallback either way. + # configure.outputs.tag is set for ANY tag, so beta (Development) tags get the DSN + # and the symbol upload below just like full releases. + COCKATRICE_SENTRY_DSN: ${{ needs.configure.outputs.tag != null && secrets.SENTRY_DSN || '' }} run: | source .ci/docker.sh args=() @@ -248,9 +261,58 @@ jobs: args+=(--ccache "$CCACHE_SIZE") args+=(--cmake-generator "$CMAKE_GENERATOR") args+=(--suffix "$SUFFIX") + # The container inherits no environment, so the DSN rides in explicitly. -e without + # =value passes it through (possibly empty); compile.sh only consumes it for Release + # configures, and empty keeps the mock fallback. + export RUN_ARGS="${RUN_ARGS:-} -e COCKATRICE_SENTRY_DSN" RUN --server --release --package "$package" "${args[@]}" + - name: "Install sentry-cli" + if: matrix.package != 'skip' && needs.configure.outputs.tag != null + # Downloads from Sentry's release registry: a Sentry outage or a broken install under + # Git Bash must not take down the release assets below. + continue-on-error: true + shell: bash + env: + SENTRY_CLI_VERSION: '3.8.0' + run: | + export INSTALL_DIR="$GITHUB_WORKSPACE/.sentry-cli" + curl -sL https://sentry.io/get-cli/ | bash + echo "$GITHUB_WORKSPACE/.sentry-cli" >> "$GITHUB_PATH" + + - name: "Upload debug symbols to Sentry" + # Like the DSN above, this runs for every tag — beta (Development) tags included. + if: matrix.package != 'skip' && needs.configure.outputs.tag != null + # A Sentry outage or rotated token must not take down the release assets below. + continue-on-error: true + shell: bash + env: + SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} + run: | + paths=() + # The binaries only, not the directories: a directory makes sentry-cli walk the + # whole build tree, where it finds (and uploads) object files and other fragments + # that can never match a stack frame. The packaged copy is split and stripped by + # cpack, but keeps this binary's GNU build-id, which is how Sentry matches it. + for app in cockatrice oracle servatrice; do + binary="build/$app/$app" + [[ -f $binary ]] && paths+=("$binary") + done + if [[ ${#paths[@]} == 0 ]]; then + # Fail loudly instead of skipping: see the Windows/macOS leg below — this step + # is the only way symbols reach Sentry and `continue-on-error` keeps the job + # green either way, so a silent skip looks exactly like a successful upload. + echo "::error::No debug symbol files found; nothing would be uploaded" + exit 1 + fi + # Project slug on Sentry; must match the project the baked DSN points at. + # No --release here: `debug-files upload` has no such option (sentry-cli errors on + # the unknown flag and uploads nothing), and debug information files are matched to + # events by debug ID via the event's debug_meta, not by release. The release grouping + # comes from the client event itself (COCKATRICE_SENTRY_RELEASE). + sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop "${paths[@]}" + # Delete used cache to emulate a ccache update. See https://github.com/actions/cache/issues/342 - name: "Delete remote compiler cache (ccache)" if: github.ref == 'refs/heads/master' && steps.restore_ccache.outputs.cache-hit @@ -536,6 +598,11 @@ jobs: BUILDTYPE: '${{ matrix.type }}' CCACHE_EVICTION_AGE: ${{ matrix.ccache_eviction_age }} CMAKE_GENERATOR: ${{ matrix.cmake_generator }} + # Tag builds only: artifacts from master pushes and PR branches must not report to + # Sentry. Forks get no secrets on pull_request, so they keep the mock fallback either way. + # configure.outputs.tag is set for ANY tag, so beta (Development) tags get the DSN + # and the symbol upload below just like full releases. + COCKATRICE_SENTRY_DSN: ${{ needs.configure.outputs.tag != null && secrets.SENTRY_DSN || '' }} DEVELOPER_DIR: '/Applications/Xcode_${{ matrix.xcode }}.app/Contents/Developer' GITHUB_TOKEN: ${{ github.token }} # needed for vcpkg dependency graph updates, see VCPKG_FEATURE_FLAGS MACOS_CERTIFICATE: ${{ secrets.PROD_MACOS_CERTIFICATE }} @@ -563,6 +630,121 @@ jobs: WITH_CRASH_REPORTING: ${{ matrix.type == 'Release' && 'ON' || '' }} run: .ci/compile.sh --server --test --vcpkg + - name: "Install sentry-cli" + if: needs.configure.outputs.tag != null && matrix.make_package == '1' + # Same as the Linux leg above: the install downloads from Sentry's registry, so a + # failure here must not block the release upload below. + continue-on-error: true + shell: bash + env: + SENTRY_CLI_VERSION: '3.8.0' + run: | + export INSTALL_DIR="$GITHUB_WORKSPACE/.sentry-cli" + curl -sL https://sentry.io/get-cli/ | bash + echo "$GITHUB_WORKSPACE/.sentry-cli" >> "$GITHUB_PATH" + + - name: "Upload debug symbols to Sentry" + # Like the DSN above, this runs for every tag — beta (Development) tags included. + if: needs.configure.outputs.tag != null && matrix.make_package == '1' + # A Sentry outage or rotated token must not take down the release assets below. + continue-on-error: true + shell: bash + env: + SENTRY_AUTH_TOKEN: ${{ secrets.SENTRY_AUTH_TOKEN }} + run: | + paths=() + if [[ '${{ matrix.os }}' == 'Windows' ]]; then + # Top-level CMakeLists.txt links /DEBUG even in release, so the linker PDBs + # land next to the binaries. Ninja is single-config, so that is + # build// and NOT build//Release/ — the same globs the symbol + # artifact step below uses. + # + # The executables are uploaded as well, in the same invocation. On 64-bit + # Windows the Call Frame Information (.pdata/.xdata) lives in the PE and is + # not copied into the PDB, so a PDB on its own leaves Sentry reporting + # "Stack Unwinding: Missing" for the image. sentry-cli also needs to see the + # PE and its PDB together: only then does it associate the PE's code + # identifier with the PDB's debug identifier. + shopt -s nullglob + for target in cockatrice oracle servatrice; do + paths+=("build/$target"/*.exe "build/$target"/*.pdb) + done + shopt -u nullglob + + # Qt's own runtime DLLs, taken from required-runtime.txt — the manifest + # .ci/compile.sh asserts the installer against — so the set uploaded is + # the set an install actually carries. The PE is the only carrier of + # unwind information for a 64-bit image, and sending it in the same + # invocation as a PDB is what lets sentry-cli associate the PE's code + # identifier with the PDB's debug identifier. Qt ships no PDBs with the + # SDK, so Qt frames resolve through the export table: names, no lines. + qt_dir= + qt_core_dir= + # Existence guards matter: the step runs under `bash -eo pipefail`, so a + # redirection-less sed over a missing file would abort the step outright + # and continue-on-error would hide it. + if [[ -f build/CMakeCache.txt ]]; then + qt_core_dir=$(sed -n 's/^Qt6Core_DIR:PATH=//p' build/CMakeCache.txt | head -1) + fi + if [[ -n $qt_core_dir ]]; then + qt_dir=$(dirname "$(dirname "$(dirname "$qt_core_dir")")") + fi + if [[ -z $qt_dir || ! -d $qt_dir ]]; then + # Warnings, not failures: the upload of our own binaries is what this + # step is for, and continue-on-error already means an upload failure + # cannot be told apart from a successful one. + echo "::warning::Qt runtime directory not resolved from build/CMakeCache.txt; Qt images get no unwind information" + elif [[ ! -f build/required-runtime.txt ]]; then + echo "::warning::build/required-runtime.txt is missing; cannot tell which Qt DLLs ship" + else + while IFS= read -r runtime; do + name=${runtime##*/} + if [[ $runtime == */* ]]; then + # A Qt plugin: the install root keeps it as plugins//. + # `|| true`: an unmatched glob makes ls exit non-zero, and pipefail + # turns that into a step abort. + runtime_path=$(ls "$qt_dir/plugins"/*/"$name" 2>/dev/null | head -1) || true + else + runtime_path="$qt_dir/bin/$name" + fi + if [[ -f ${runtime_path:-} ]]; then + paths+=("$runtime_path") + else + echo "::warning::shipped Qt runtime $runtime not found under $qt_dir" + fi + done < <(grep -E '^(Qt6[^/]*\.dll|Plugins/)' build/required-runtime.txt) + fi + else + # Release builds carry DWARF (-g, kept by CrashReporting.cmake); macOS keeps its + # debug info in dSYM bundles, so materialize them before uploading. + for app in cockatrice oracle servatrice; do + binary="build/$app/$app.app/Contents/MacOS/$app" + if [[ -f $binary ]]; then + if [[ '${{ matrix.os }}' == 'macOS' ]]; then + dsymutil "$binary" -o "$binary.dSYM" + paths+=("$binary.dSYM") + # The dSYM holds the debug information, but the unwind information + # Sentry needs to walk a minidump stack lives in the Mach-O itself. + paths+=("$binary") + else + paths+=("$binary") + fi + fi + done + fi + if [[ ${#paths[@]} == 0 ]]; then + # Fail loudly instead of skipping: this step is the only way symbols reach + # Sentry, and `continue-on-error` keeps the job green either way, so a + # silent skip is indistinguishable from a successful upload. It also hid a + # wrong Windows path (build//Release, which Ninja never creates). + echo "::error::No debug symbol files found; nothing would be uploaded" + exit 1 + fi + # Project slug on Sentry; must match the project the baked DSN points at. + # No --release: see the Linux leg above — the flag does not exist on + # `debug-files upload` and DIFs are matched by debug ID, not release. + sentry-cli debug-files upload --org cockatrice --project cockatrice-desktop "${paths[@]}" + # CPack reports a failed makensis run as "Problem running NSIS command" # plus a path to NSISOutput.log, and that log lives only on the runner, so # the actual compile error is otherwise nowhere in the workflow output. @@ -706,19 +888,6 @@ jobs: if-no-files-found: error path: ${{ steps.build.outputs.path }} - # Ninja is single-config: binaries and their PDBs land directly in the - # target directories instead of a Release subdirectory. - - name: "[Windows] Upload PDBs (Program Databases)" - if: matrix.os == 'Windows' && github.ref_type != 'tag' - uses: actions/upload-artifact@v7 - with: - if-no-files-found: error - name: ${{ steps.build.outputs.name }}-PDBs - path: | - build/cockatrice/*.pdb - build/oracle/*.pdb - build/servatrice/*.pdb - - name: "Upload to release" if: needs.configure.outputs.tag != null && matrix.make_package == '1' id: upload_release @@ -730,6 +899,33 @@ jobs: tag_name: ${{ needs.configure.outputs.tag }} run: gh release upload "$tag_name" "$asset_path#$asset_name" + # Ninja is single-config: binaries and their PDBs land directly in the + # target directories instead of a Release subdirectory. + # + # Archived for tag builds only, and ordered after the release upload so + # a missing PDB can never block a release from being published. On a tag + # build this artifact is the only durable copy of the symbols: the Sentry + # upload is continue-on-error, so without it a failed upload would lose + # the ability to symbolicate that release permanently. Other builds upload + # no symbols at all, so archiving them there would only cost storage. + # + # The executables travel with their PDBs: re-uploading the pair together is + # what lets sentry-cli associate the PE's code identifier with the PDB's + # debug identifier, and the PE is what carries the unwind information. + - name: "[Windows] Upload debug symbols (exe + pdb)" + if: matrix.os == 'Windows' && needs.configure.outputs.tag != null + uses: actions/upload-artifact@v7 + with: + if-no-files-found: error + name: ${{ steps.build.outputs.name }}-Symbols + path: | + build/cockatrice/*.exe + build/cockatrice/*.pdb + build/oracle/*.exe + build/oracle/*.pdb + build/servatrice/*.exe + build/servatrice/*.pdb + - name: "Attest binary provenance" if: steps.upload_release.outcome == 'success' id: attestation diff --git a/cmake/CrashReporting.cmake b/cmake/CrashReporting.cmake index abd029cc2..db016269f 100644 --- a/cmake/CrashReporting.cmake +++ b/cmake/CrashReporting.cmake @@ -46,12 +46,14 @@ if(WITH_CRASH_REPORTING AND WITH_CLIENT) if(ENABLE_SANITIZERS) # The crash handler cannot run under ASan/UBSan, so there is no backend to capture with. set(_sentry_backend "none") - elseif(WIN32 OR APPLE) - # Out-of-process handler; shipping the handler binary is a later increment, which unifies - # every OS on breakpad (in-process, no extra binary to ship). - set(_sentry_backend "crashpad") else() - # In-process, no extra binary to ship. + # Breakpad on every OS (increment 3 probe, 2026-10-14): the crashpad sources + # do vendor friction-free (FetchContent inits all submodules recursively by + # default, verified populated in a local build tree), but the crashpad backend + # would need its out-of-process crashpad_handler binary installed next to the + # exe, into the macOS bundle, and through NSIS, plus handler-path wiring and + # notarization care. Breakpad is in-process, needs no extra binary and no + # packaging on any OS sentry-native builds, so simplicity wins. set(_sentry_backend "breakpad") endif() # FORCE so toggling ENABLE_SANITIZERS on a reused build dir cannot leave a stale backend behind. @@ -61,6 +63,18 @@ if(WITH_CRASH_REPORTING AND WITH_CLIENT) ) unset(_sentry_backend) + if(NOT MSVC) + # Symbol upload needs debug info in Release binaries: the top-level flags build GCC + # Release as -s -O2 (stripped, no DWARF) and Clang/Apple Release as plain -O2 (no DWARF), + # both of which upload nothing useful. Keep -g and drop the strip flag while the SDK is + # on; the OFF default leaves local, distro and Debug builds untouched. MSVC already + # links /DEBUG with PDBs in release (see top-level CMakeLists.txt), so it needs nothing. + string(REGEX REPLACE "(^| )-s( |$)" "\\1\\2" CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE}") + if(NOT CMAKE_CXX_FLAGS_RELEASE MATCHES "(^| )-g( |$)") + set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} -g") + endif() + endif() + # No Qt integration: it runs find_package(Qt6) inside the vendored build, which # cannot see our thin/aqt Qt layouts (macOS CI) under foreign toolchains, and its # breadcrumbs could carry unredacted log lines into native crash events that bypass @@ -129,3 +143,39 @@ else() "(WITH_CRASH_REPORTING=${WITH_CRASH_REPORTING}, WITH_CLIENT=${WITH_CLIENT})" ) endif() + +# Packaged release binaries must not ship the DWARF that the symbol upload needs: it is +# 80-90% of an unstripped binary and nobody downloads a 500 MB installer to get it. The +# build tree keeps -g (CI uploads symbols from there); the *packaged copy* is split at +# install time by a CPack install script, which extracts the debug info next to the staging +# dir and strips the binary. Only ELF (Linux/BSD) needs this: Windows links /DEBUG with +# separate PDBs, and macOS keeps its dSYM extracted from the bundle. +option(COCKATRICE_SPLIT_DEBUG_SYMBOLS "Strip DWARF out of packaged release binaries" ON) +set(COCKATRICE_DEBUG_SYMBOL_DIR + "${CMAKE_BINARY_DIR}/debug-symbols" + CACHE PATH "Where the packaged binaries' split debug information is written (kept out of the package)" +) +if(WITH_CRASH_REPORTING + AND WITH_CLIENT + AND COCKATRICE_SPLIT_DEBUG_SYMBOLS + AND CMAKE_BUILD_TYPE STREQUAL "Release" + AND UNIX + AND NOT APPLE +) + find_program(COCKATRICE_OBJCOPY NAMES objcopy) + find_program(COCKATRICE_STRIP_TOOL NAMES strip) + if(COCKATRICE_OBJCOPY AND COCKATRICE_STRIP_TOOL) + configure_file( + "${CMAKE_CURRENT_LIST_DIR}/SplitDebugSymbols.cmake.in" "${CMAKE_BINARY_DIR}/cmake/SplitDebugSymbols.cmake" @ONLY + ) + # PRE_BUILD runs after CPack stages the install tree and before the package is produced. + # CPACK_INSTALL_SCRIPTS would run too early (staging is still empty); the script walks + # CPACK_TEMPORARY_DIRECTORY, never CMAKE_INSTALL_PREFIX, and refuses to run otherwise. + list(APPEND CPACK_PRE_BUILD_SCRIPTS "${CMAKE_BINARY_DIR}/cmake/SplitDebugSymbols.cmake") + message(STATUS "Crash reporting: packaged binaries stripped, debug info split to ${COCKATRICE_DEBUG_SYMBOL_DIR}") + else() + message(WARNING "COCKATRICE_SPLIT_DEBUG_SYMBOLS is ON but objcopy/strip were not found; " + "packaged release binaries will keep their debug information" + ) + endif() +endif() diff --git a/cmake/NSIS.template.in b/cmake/NSIS.template.in index 811a3cb7f..3647303b9 100644 --- a/cmake/NSIS.template.in +++ b/cmake/NSIS.template.in @@ -1530,11 +1530,22 @@ ${If} $PortableMode = 0 ${GetSize} "$INSTDIR" "/S=0K" $0 $1 $2 IntFmt $0 "0x%08X" $0 - ; Enable Windows User-Mode Dumps + ; No Windows Error Reporting LocalDumps registration: crash capture belongs to + ; the in-app reporter (Sentry/breakpad, MiniDumpNormal-class data only). Opting + ; machines into OS-level full dumps (DumpType 2) would leave unredacted memory + ; dumps on user machines that nothing consumes. ; https://learn.microsoft.com/en-us/windows/win32/wer/collecting-user-mode-dumps - WriteRegExpandStr HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" "DumpFolder" "%LOCALAPPDATA%\CrashDumps\Cockatrice" - WriteRegDWORD HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" "DumpCount" "5" - WriteRegDWORD HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" "DumpType" "2" + + ; Older installers did register cockatrice.exe above (and the removed + ; MiniDumpWithFullMemory writer filled that folder), and that key survives this + ; install, so WER would keep writing full dumps. Remove it, and clear the + ; legacy dumps nothing consumes anymore. $LOCALAPPDATA resolves per shell context: + ; under "all" it points at ProgramData, so switch to "current" for the per-user + ; dumps folder and back afterwards; the registry delete works in either context. + DeleteRegKey HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" + SetShellVarContext current + RMDir /r "$LOCALAPPDATA\CrashDumps\Cockatrice" + SetShellVarContext all WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" "DisplayIcon" "$INSTDIR\cockatrice.exe" WriteRegStr HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" "DisplayName" "Cockatrice" @@ -1635,6 +1646,15 @@ Section "un.Application" UnSecApplication DeleteRegKey HKLM "Software\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" + ; Leftover WER LocalDumps key from pre-Sentry installers (see the installer note + ; above): removing it here covers machines that uninstall without reinstalling. + ; The legacy full-memory dumps go with it; nothing consumes them anymore. Same shell + ; context note as the installer: the per-user dumps folder needs "current". + DeleteRegKey HKLM "Software\Microsoft\Windows\Windows Error Reporting\LocalDumps\cockatrice.exe" + SetShellVarContext current + RMDir /r "$LOCALAPPDATA\CrashDumps\Cockatrice" + SetShellVarContext all + ; Only remove the file/protocol associations if we registered them (i.e. the ; install was not portable) and .cod is still owned by Cockatrice, so we don't ; clobber a .cod association installed by another application. diff --git a/cmake/SplitDebugSymbols.cmake.in b/cmake/SplitDebugSymbols.cmake.in new file mode 100644 index 000000000..bfe954712 --- /dev/null +++ b/cmake/SplitDebugSymbols.cmake.in @@ -0,0 +1,80 @@ +# Splits DWARF out of the packaged ELF binaries, then strips them. +# +# This is a CPack pre-build script (CPACK_PRE_BUILD_SCRIPTS, wired in CrashReporting.cmake): +# CPack runs it after staging the install tree and before producing the package, so it only +# ever touches the copy that becomes the release artifact. The build tree is left alone: -g +# stays on, and CI uploads symbols to Sentry straight from build/. +# +# For every ELF file under CPACK_TEMPORARY_DIRECTORY the script extracts the debug information +# to COCKATRICE_DEBUG_SYMBOL_DIR and removes the debug sections from the packaged binary. +# COCKATRICE_DEBUG_SYMBOL_DIR is deliberately outside the staging tree, so sidecars never end +# up in the package; CI can archive/upload them if wanted (it normally uploads straight from +# the build tree). The only thing that must survive stripping is .eh_frame (allocatable, used +# for unwinding), which neither --only-keep-debug nor --strip-debug removes. +# +# Tool paths, the symbol directory and the tree to walk are baked in / provided at configure / +# run time; the guards below refuse to run on anything that is not a CPack staging directory, +# so a missing variable can never turn the glob into a filesystem-wide walk. + +set(_objcopy "@COCKATRICE_OBJCOPY@") +set(_strip "@COCKATRICE_STRIP_TOOL@") +set(_symbolDir "@COCKATRICE_DEBUG_SYMBOL_DIR@") + +if(_objcopy STREQUAL "" OR _strip STREQUAL "") + message(WARNING "split debug symbols: objcopy/strip not found at configure time, packaging unstripped binaries") + return() +endif() +if(_symbolDir STREQUAL "") + message(FATAL_ERROR "split debug symbols: COCKATRICE_DEBUG_SYMBOL_DIR is empty") +endif() + +# CPack sets CPACK_TEMPORARY_DIRECTORY to the staging tree; CMAKE_INSTALL_PREFIX is still the +# configured /usr prefix here, so globbing it would walk system directories. Refuse to run +# unless the path exists and really is a CPack staging tree. +if(NOT CPACK_TEMPORARY_DIRECTORY OR NOT IS_DIRECTORY "${CPACK_TEMPORARY_DIRECTORY}") + message(STATUS "split debug symbols: no CPack staging directory, skipping") + return() +endif() +if(NOT CPACK_TEMPORARY_DIRECTORY MATCHES "_CPack_Packages") + message(WARNING "split debug symbols: ${CPACK_TEMPORARY_DIRECTORY} is not a CPack staging tree, skipping") + return() +endif() + +file(MAKE_DIRECTORY "${_symbolDir}") + +file( + GLOB_RECURSE _entries + LIST_DIRECTORIES true + "${CPACK_TEMPORARY_DIRECTORY}/*" +) +set(_processed 0) +foreach(_file IN LISTS _entries) + if(IS_DIRECTORY "${_file}" OR IS_SYMLINK "${_file}") + continue() + endif() + # Only ELF: objcopy would fail on the icons, translations and Qt resources next to it. + file( + READ "${_file}" _magic + OFFSET 0 + LIMIT 4 + HEX + ) + if(NOT _magic STREQUAL "7f454c46") + continue() + endif() + get_filename_component(_name "${_file}" NAME) + set(_debug "${_symbolDir}/${_name}.debug") + execute_process(COMMAND "${_objcopy}" --only-keep-debug "${_file}" "${_debug}" RESULT_VARIABLE _rc) + if(NOT _rc EQUAL 0) + message(WARNING "split debug symbols: objcopy --only-keep-debug failed for ${_file}") + continue() + endif() + execute_process(COMMAND "${_strip}" --strip-debug --strip-unneeded "${_file}" RESULT_VARIABLE _rc) + if(NOT _rc EQUAL 0) + message(WARNING "split debug symbols: strip failed for ${_file}") + continue() + endif() + math(EXPR _processed "${_processed} + 1") +endforeach() + +message(STATUS "Split debug symbols for ${_processed} packaged ELF file(s); sidecars in ${_symbolDir}") diff --git a/cockatrice/src/client/crash/crash_reporter.cpp b/cockatrice/src/client/crash/crash_reporter.cpp index 720bcd350..07a0389c7 100644 --- a/cockatrice/src/client/crash/crash_reporter.cpp +++ b/cockatrice/src/client/crash/crash_reporter.cpp @@ -5,12 +5,15 @@ #endif #include +#include #include #include #include #include +#include #include #include +#include #include #include #include @@ -263,8 +266,16 @@ void CrashReporter::init(const QString &dsn, sentry_options_set_release(options, release.toUtf8().constData()); sentry_options_set_database_path(options, sentryDatabasePath.toUtf8().constData()); // Consent-gated uploads: crashes are captured into the local database but - // nothing leaves the machine until giveConsent(). + // nothing leaves the machine until giveConsent(). cache_keep holds the previous run's + // crash envelope in /cache instead of dropping it at init, so the Ask dialog + // decides its fate: giveConsent() flushes it via the transport retry, revokeConsent() + // deletes it below. This also covers the first crash after switching to AlwaysSend, + // which giveConsent() (running after init) would otherwise miss. http_retry is required + // for the flush: without it the held envelope is written with a plain filename the retry + // scanner skips, and giveConsent()'s sentry_transport_retry() sends nothing. sentry_options_set_require_user_consent(options, 1); + sentry_options_set_cache_keep(options, SENTRY_CACHE_KEEP_OFFLINE); + sentry_options_set_http_retry(options, 1); sentry_options_set_before_send(options, scrubEventBeforeSend, nullptr); ready = (sentry_init(options) == 0); #else @@ -291,6 +302,79 @@ void CrashReporter::shutdown() ready = false; } +CrashReporter::~CrashReporter() = default; +CrashReporter::CrashReporter() = default; + +bool CrashReporter::beginSession(const QString &dir) +{ + // Restartable: releasing first keeps begin->end->begin round-trips clean. + endSession(); + + QDir().mkpath(dir); + const QDir sessionDir(dir); + const QString selfName = QStringLiteral("session-%1").arg(QCoreApplication::applicationPid()); + + bool unclean = false; + const QStringList leftovers = sessionDir.entryList({QStringLiteral("session-*")}, QDir::Files); + for (const QString &name : leftovers) { + if (name == selfName) { + continue; + } + const QString path = sessionDir.filePath(name); + QLockFile probe(path); + // Long-lived session lock: stale means "owner dead", regardless of file age, so a + // crash followed by an immediate relaunch (younger than the 30 s default) is still + // detected. tryLock() only deletes the file when it finds it stale, so a live + // concurrent instance's lock is never touched here. (The static + // QLockFile::removeStaleLockFile(path) form does not exist in Qt5/Qt6, and the + // member removeStaleLockFile() force-removes without a staleness check, so neither + // is usable for this scan.) + probe.setStaleLockTime(0); + if (probe.tryLock(0)) { + unclean = true; + probe.unlock(); // deletes the stale file + continue; + } + // tryLock failed: either a live concurrent instance owns the file, or it is + // unreadable (garbage/empty) and therefore has no live owner. Only the latter is + // cleaned up; a live instance is left alone. On Unix QLockFile creates the file + // before writing the pid, so a second instance starting at the same moment can + // briefly observe an empty (or half-written) file for a live owner: re-read once + // after a beat so a live owner's fresh lock is never mistaken for garbage and deleted. + qint64 pid = 0; + QString hostname, appname; + if (!probe.getLockInfo(&pid, &hostname, &appname)) { + QThread::msleep(100); + if (!probe.getLockInfo(&pid, &hostname, &appname)) { + QFile::remove(path); + unclean = true; + } + } + } + + sessionLockPath = sessionDir.filePath(selfName); + sessionLock = std::make_unique(sessionLockPath); + sessionLock->setStaleLockTime(0); + if (!sessionLock->tryLock(0)) { + qCWarning(CrashReportLog) << "Failed to claim session lock at" << sessionLockPath; + } + return unclean; +} + +void CrashReporter::endSession() +{ + if (sessionLock) { + // unlock() removes the lock file; nothing left to clean up. + sessionLock->unlock(); + sessionLock.reset(); + } else if (!sessionLockPath.isEmpty()) { + // The lock was never claimed (a live concurrent instance owns its file): only our + // own unclaimed file, if tryLock left one behind, is removed. + QFile::remove(sessionLockPath); + } + sessionLockPath.clear(); +} + void CrashReporter::giveConsent() { #if COCKATRICE_WITH_CRASH_REPORTING @@ -307,6 +391,10 @@ void CrashReporter::revokeConsent() // return to unknown, which blocks uploads too but loses the decision. if (ready) { sentry_user_consent_revoke(); + // Drop the held crash envelope: without this it would sit in /cache and + // upload on the next give (e.g. after switching to AlwaysSend), after the user + // explicitly refused it. The SDK recreates the directory as needed. + QDir(sentryDatabasePath + QStringLiteral("/cache")).removeRecursively(); } #endif } diff --git a/cockatrice/src/client/crash/crash_reporter.h b/cockatrice/src/client/crash/crash_reporter.h index 598b6445c..aecc6ade3 100644 --- a/cockatrice/src/client/crash/crash_reporter.h +++ b/cockatrice/src/client/crash/crash_reporter.h @@ -11,6 +11,7 @@ #include #include #include +#include #ifndef COCKATRICE_WITH_CRASH_REPORTING #define COCKATRICE_WITH_CRASH_REPORTING 0 @@ -24,31 +25,46 @@ #define COCKATRICE_SENTRY_RELEASE "" #endif +class QLockFile; + Q_DECLARE_LOGGING_CATEGORY(CrashReportLog); /** * @brief Sentry-backed crash-report facade, owned by main(). * - * Single responsibility: init/shutdown the sentry-native SDK behind consent - * gating, plus the shared log redaction helpers. An empty DSN (or Never - * consent) skips the SDK entirely, so the mock dialog fallback behaves exactly - * as before. With a DSN, init() arms require-user-consent and before_send - * scrubbing (stale persisted consent from a previous AlwaysSend run is cleared - * before init when the mode is not AlwaysSend); giveConsent()/revokeConsent() - * gate uploads. attachToPendingCrashEnvelopes() folds the redacted log and the - * optional user note into the pending native crash event(s) before consent - * releases them, so dump, stack, log and note arrive as ONE Sentry event; - * sendReport() is the fallback for unclean exits that produced no native crash - * event, capturing a redacted message event with the log preview attached. All - * SDK calls are guarded so OFF builds compile to no-ops. + * An empty DSN (or Never consent) skips the SDK entirely, so the mock dialog + * fallback behaves exactly as before. All SDK calls are guarded so OFF builds + * compile to no-ops. + * + * @par Consent gating + * init() arms require-user-consent and before_send scrubbing. Stale persisted + * consent from a previous AlwaysSend run is cleared before init when the mode is + * not AlwaysSend, and cached crash envelopes are held for the dialog instead of + * being dropped. giveConsent()/revokeConsent() gate uploads. + * + * @par Envelope and log helpers + * attachToPendingCrashEnvelopes() folds the redacted log and the optional user + * note into the pending native crash event(s) before consent releases them, so + * dump, stack, log and note arrive as ONE Sentry event. sendReport() is the + * fallback for unclean exits that produced no native crash event, capturing a + * redacted message event with the log preview attached. The log redaction + * helpers are static and SDK-independent. + * + * @par Session tracking + * beginSession()/endSession() are SDK-independent: a QLockFile per process in + * the database dir detects unclean exits cross-platform, replacing the legacy + * Windows-only .dmp scan. The held lock makes the reporter non-copyable and + * non-movable. */ class CrashReporter { public: - CrashReporter() = default; + CrashReporter(); CrashReporter(const CrashReporter &) = delete; CrashReporter &operator=(const CrashReporter &) = delete; - ~CrashReporter() = default; + CrashReporter(CrashReporter &&) = delete; + CrashReporter &operator=(CrashReporter &&) = delete; + ~CrashReporter(); void init(const QString &dsn, const QString &release, @@ -140,12 +156,22 @@ public: */ static QString describeOriginMismatch(const QString &envelopePath, const QString ¤tDsn); + /** + * @brief Claim this process's session lock, reporting a previous unclean exit. + * @return true if a leftover session lock from a dead process (or an unreadable + * garbage file) was found and cleaned; live concurrent instances are ignored. + */ + bool beginSession(const QString &dir); + void endSession(); + static QString sanitizeLogLine(const QString &line); static QString redactCockatriceUrls(const QString &line); private: bool ready = false; QString sentryDatabasePath; + QString sessionLockPath; + std::unique_ptr sessionLock; }; #endif diff --git a/cockatrice/src/main.cpp b/cockatrice/src/main.cpp index fd98b1945..7b8648d69 100644 --- a/cockatrice/src/main.cpp +++ b/cockatrice/src/main.cpp @@ -46,9 +46,6 @@ #include #include #include -#include -#include -#include #include #include #include @@ -108,60 +105,6 @@ static void CockatriceLogger(QtMsgType type, const QMessageLogContext &ctx, cons Logger::getInstance().log(type, ctx, logMessage); } -#ifdef Q_OS_WIN -// clang-format off -#include -#include -#include -#include -#include - -#pragma comment(lib, "DbgHelp.lib") // Link the DbgHelp library -// clang-format on - -LONG WINAPI CockatriceUnhandledExceptionFilter(EXCEPTION_POINTERS *exceptionPointers) -{ - std::filesystem::path path; - - // Find %LOCALAPPDATA% (or cheat at finding it) - wchar_t *localAppDataFolder; - if (SHGetKnownFolderPath(FOLDERID_LocalAppData, KF_FLAG_CREATE, NULL, &localAppDataFolder) != S_OK) { - path = std::filesystem::temp_directory_path().parent_path().parent_path(); - } else { - path = std::filesystem::path(localAppDataFolder); - } - - // Plan on writing crash files into %LOCALAPPDATA%/CrashDumps/Cockatrice/cockatrice.crash.*.dmp - path /= "CrashDumps"; - path /= "Cockatrice"; - if (!std::filesystem::exists(path)) { - std::filesystem::create_directories(path); - } - path /= "cockatrice.crash." + std::to_string(std::time(0)) + ".dmp"; - - // Create and write crash files -#ifdef UNICODE - HANDLE hDumpFile = - CreateFile(path.wstring().c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr); -#else - HANDLE hDumpFile = - CreateFile(path.string().c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr); -#endif - - MINIDUMP_EXCEPTION_INFORMATION mei; - mei.ExceptionPointers = exceptionPointers; - mei.ThreadId = GetCurrentThreadId(); - mei.ClientPointers = 1; - - MiniDumpWriteDump(GetCurrentProcess(), GetCurrentProcessId(), hDumpFile, MiniDumpWithFullMemory, &mei, nullptr, - nullptr); - - CloseHandle(hDumpFile); - - return EXCEPTION_EXECUTE_HANDLER; -} -#endif - void installNewTranslator() { QString lang = SettingsCache::instance().personal().getLang(); @@ -202,11 +145,52 @@ static QString redactActivationUrl(const QString &url) return parsed.scheme() + "://" + parsed.host(); } +/** + * @brief Run the startup crash-report flow for an unclean exit (or --simulate-crash). + * + * With a configured DSN the SDK owns capture and upload (consent-gated); with an + * empty DSN the flow falls back to a local mock file. AlwaysSend uploads silently + * (the log was folded into the pending native envelope(s) at startup); Ask offers + * the dialog; NeverSend skips silently. + * + * @param attachedEnvelopes number of pending native envelopes that already carry the + * redacted log, so an AlwaysSend run only sends a standalone + * log event when no crash event existed. + */ +static void runCrashReportFlow(CrashReporter &crashReporter, QWidget *parent, int attachedEnvelopes) +{ + const PersonalSettings::CrashReportingMode crashMode = SettingsCache::instance().personal().getCrashReportingMode(); + const bool crashReady = crashReporter.isReady(); + if (crashMode == PersonalSettings::CrashReportingMode::AlwaysSend) { + if (crashReady) { + // Silent path: the redacted log was folded into the pending native crash + // envelope(s) at startup and consent already released them. Only when no + // pending crash event existed (e.g. --simulate-crash) does a standalone + // log event still need to go out here. + if (attachedEnvelopes == 0) { + const QString crashPreview = DlgCrashReport::buildRedactedPreview(); + if (!crashReporter.sendReport(crashPreview)) { + const QString savedPath = DlgCrashReport::writeMockReport(crashPreview); + qCWarning(MainLog) << "Failed to send crash report, saved locally to" << savedPath; + } + } + } else { + DlgCrashReport::writeMockReport(DlgCrashReport::buildRedactedPreview()); + } + } else if (crashMode == PersonalSettings::CrashReportingMode::Ask) { + // Real mode: Send gives consent + captures the event, Don't send revokes + // consent; the dialog owns both via the reporter. + DlgCrashReport crashDialog(parent, crashReady, crashReady ? &crashReporter : nullptr); + crashDialog.exec(); + } + // NeverSend: skip silently (no dialog, nothing written or sent). +} + int main(int argc, char *argv[]) { -#ifdef Q_OS_WIN - SetUnhandledExceptionFilter(CockatriceUnhandledExceptionFilter); -#endif + // No custom unhandled-exception filter: sentry owns native crash capture on all + // platforms. (The legacy MiniDumpWithFullMemory writer was removed as a privacy + // liability; unclean exits are detected via CrashReporter::beginSession below.) // Logging setup #ifdef Q_OS_APPLE @@ -404,6 +388,16 @@ int main(int argc, char *argv[]) } } + // Unclean-exit detection (cross-platform QLockFile session tracking, replacing the + // legacy Windows-only .dmp scan): a leftover session lock means the previous run died + // without reaching endSession(). Our own lock is held until the event loop finishes. + // Deliberately placed after parser.process() (--help/--version exit inside it and must + // not leak a lock) and after the instance hand-off (a handed-off secondary exits + // immediately and must never take a lock). Note this counts any death without + // endSession() as a crash, including SIGTERM at logout, Task Manager kills and + // session-end shutdowns; in AlwaysSend those send a report too. + const bool uncleanExit = crashReporter.beginSession(crashDatabasePath); + rng = new RNG_SFMT(CryptoUtil::randomUInt64()); themeManager = new ThemeManager; soundEngine = new SoundEngine; @@ -497,63 +491,11 @@ int main(int argc, char *argv[]) ui.show(); qCInfo(MainLog) << "ui.show() finished"; - // Crash-report flow: with a configured DSN the SDK owns capture and upload - // (consent-gated); with an empty DSN the dialog falls back to a local mock - // file and behaves exactly as before. Triggered by --simulate-crash or by - // leftover Windows crash dumps, which are consumed after handling so one - // crash never prompts twice. - { - QString crashDumpDir; -#ifdef Q_OS_WIN - // Mirrors the writer (CockatriceUnhandledExceptionFilter): it appends CrashDumps/Cockatrice - // to %LOCALAPPDATA%, or, when its known-folder lookup fails, to the temp directory walked up - // two parent_path() steps. %LOCALAPPDATA% is this scan's stand-in for FOLDERID_LocalAppData: - // both come from the same profile, so when the lookup fails the variable is normally unset. - QString crashDumpRootPath = QString::fromLocal8Bit(qgetenv("LOCALAPPDATA")); - if (crashDumpRootPath.isEmpty()) { - // The writer's fallback expression, verbatim — QDir::cdUp() is existence-checked and - // would silently leave the path alone here, parent_path() is not. - const std::filesystem::path fallback = std::filesystem::temp_directory_path().parent_path().parent_path(); - crashDumpRootPath = QString::fromStdWString(fallback.wstring()); - } - crashDumpDir = QDir(crashDumpRootPath).absoluteFilePath(QStringLiteral("CrashDumps/Cockatrice")); -#else - crashDumpDir = QDir::tempPath(); -#endif - const QStringList crashDumps = - QDir(crashDumpDir).entryList({QStringLiteral("cockatrice.crash.*.dmp")}, QDir::Files); - if (parser.isSet("simulate-crash") || !crashDumps.isEmpty()) { - const PersonalSettings::CrashReportingMode crashMode = - SettingsCache::instance().personal().getCrashReportingMode(); - const bool crashReady = crashReporter.isReady(); - if (crashMode == PersonalSettings::CrashReportingMode::AlwaysSend) { - if (crashReady) { - // Silent path: the redacted log was folded into the pending native crash - // envelope(s) at startup and consent already released them. Only when no - // pending crash event existed (e.g. --simulate-crash) does a standalone - // log event still need to go out here. - if (crashLogAttachedEnvelopes == 0) { - const QString crashPreview = DlgCrashReport::buildRedactedPreview(); - if (!crashReporter.sendReport(crashPreview)) { - const QString savedPath = DlgCrashReport::writeMockReport(crashPreview); - qCWarning(MainLog) << "Failed to send crash report, saved locally to" << savedPath; - } - } - } else { - DlgCrashReport::writeMockReport(DlgCrashReport::buildRedactedPreview()); - } - } else if (crashMode == PersonalSettings::CrashReportingMode::Ask) { - // Real mode: Send gives consent + captures the event, Don't - // send revokes consent; the dialog owns both via the reporter. - DlgCrashReport crashDialog(&ui, crashReady, crashReady ? &crashReporter : nullptr); - crashDialog.exec(); - } - // NeverSend: skip silently (no dialog, nothing written or sent). - // Handled dumps are consumed so one crash never prompts twice. - for (const QString &dump : crashDumps) { - QFile::remove(crashDumpDir + QStringLiteral("/") + dump); - } - } + // Crash-report flow: the trigger is --simulate-crash or an unclean previous exit + // (leftover session lock; the old %LOCALAPPDATA%/CrashDumps .dmp scan is gone with + // the exception filter). See runCrashReportFlow() for the per-mode handling. + if (parser.isSet("simulate-crash") || uncleanExit) { + runCrashReportFlow(crashReporter, &ui, crashLogAttachedEnvelopes); } // force shortcuts to be shown/hidden in right-click menus, regardless of system defaults @@ -580,8 +522,6 @@ int main(int argc, char *argv[]) int ret = app.exec(); - crashReporter.shutdown(); - qCInfo(MainLog) << "Event loop finished, terminating..."; delete rng; PingPixmapGenerator::clear(); @@ -589,5 +529,10 @@ int main(int argc, char *argv[]) UserLevelPixmapGenerator::clear(); ManaSymbolPixmapGenerator::clear(); + // Released last: crashes anywhere in the teardown above (a common place for Qt apps to + // crash) are still captured by the SDK and detected as unclean exits on the next launch. + crashReporter.shutdown(); + crashReporter.endSession(); + return ret; } diff --git a/tests/crash_reporter_test.cpp b/tests/crash_reporter_test.cpp index e15007f7a..e44a34a60 100644 --- a/tests/crash_reporter_test.cpp +++ b/tests/crash_reporter_test.cpp @@ -1,10 +1,14 @@ #include "client/crash/crash_reporter.h" #include +#include #include #include +#include #include +#include #include +#include #include #include #include @@ -339,6 +343,81 @@ TEST(CrashReporterTest, AttachSkipsNoteWhenEmptyAndFindsRunDirEnvelopes) ASSERT_FALSE(content.contains(QByteArrayLiteral("user-note.txt"))); } +TEST(CrashReporterTest, BeginSessionOnEmptyDirReportsClean) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + CrashReporter reporter; + ASSERT_FALSE(reporter.beginSession(dir.path())); + reporter.endSession(); +} + +TEST(CrashReporterTest, BeginSessionIgnoresLiveConcurrentInstance) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + // A second QLockFile held in-process stands in for a live concurrent instance: same + // pid, alive, so the scan must leave it alone and report a clean start. + QLockFile liveInstance(dir.filePath(QStringLiteral("session-424242"))); + ASSERT_TRUE(liveInstance.tryLock(0)); + CrashReporter reporter; + ASSERT_FALSE(reporter.beginSession(dir.path())); + ASSERT_TRUE(QFile::exists(liveInstance.fileName())); + reporter.endSession(); + liveInstance.unlock(); +} + +TEST(CrashReporterTest, BeginSessionDetectsStaleDeadOwnerLock) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + // A well-formed lock whose owner is dead: our own pid (so the host matches) but a + // process name that cannot match, which is how QLockFile spots pid reuse. Must be + // reported unclean and cleaned up, even though the file is brand new (this is why + // the scan probes with staleLockTime 0 rather than the 30 s default). + const QString stalePath = dir.filePath(QStringLiteral("session-stale-owner")); + QFile stale(stalePath); + ASSERT_TRUE(stale.open(QIODevice::WriteOnly | QIODevice::Text)); + QTextStream out(&stale); + out << QCoreApplication::applicationPid() << '\n' + << QStringLiteral("definitely-not-the-test-binary") << '\n' + << QSysInfo::machineHostName() << '\n'; + stale.close(); + + CrashReporter reporter; + ASSERT_TRUE(reporter.beginSession(dir.path())); + ASSERT_FALSE(QFile::exists(stalePath)); + reporter.endSession(); +} + +TEST(CrashReporterTest, BeginSessionCleansGarbageFileAndReportsUnclean) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + // An unreadable lock file has no live owner by definition: report unclean and remove it. + const QString garbagePath = dir.filePath(QStringLiteral("session-garbage")); + QFile garbage(garbagePath); + ASSERT_TRUE(garbage.open(QIODevice::WriteOnly)); + garbage.write("not-a-lock-file\x00\xff", 17); + garbage.close(); + + CrashReporter reporter; + ASSERT_TRUE(reporter.beginSession(dir.path())); + ASSERT_FALSE(QFile::exists(garbagePath)); + reporter.endSession(); +} + +TEST(CrashReporterTest, BeginSessionEndSessionRoundTripReportsClean) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + CrashReporter reporter; + ASSERT_FALSE(reporter.beginSession(dir.path())); + reporter.endSession(); + ASSERT_FALSE(reporter.beginSession(dir.path())); + reporter.endSession(); +} + } // namespace int main(int argc, char **argv) From c6f682f9cf43996a4e6ce141dae5b078d97dac9b Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Mon, 19 Oct 2026 14:57:04 +0200 Subject: [PATCH 04/90] [CrashReporting] Offer to send the installer log on abort (#7524) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [CrashReporting] Installer log send offer on abort (inc 4) DSN parsed to CPACK_SENTRY_* (scheme-aware for http self-hosts) before include(CPack); new SendInstallerLog.ps1.in builds a byte-accurate Sentry envelope (128 KiB tail cap, X-Sentry-Auth, exit 0/1) with the abort reason read from the log file itself, so the NSIS side needs no string functions and quoting stays trivial. AbortInstall asks consent (attended installs only, silent path untouched, abort never swallowed). Empty/malformed DSN disables the offer; no secrets in repo. * [CrashReporting] Embed installer-log sender via a relative NSIS path The AbortInstall hook embedded SendInstallerLog.ps1 through the absolute CPACK_SENTRY_PS1_PATH, which carries CMake's forward slashes (D:/a/...). makensis does not treat a forward-slash drive path as absolute, so it resolved it relative to project.nsi, found nothing and aborted packaging on the Windows jobs of every branch in the stack. Embed it with the same '..\..\..\' relative path that already resolves NSIS.definitions.nsh: both are configure_file'd into the build root, three levels up from where CPack places project.nsi. Drop the now-unused CPACK_SENTRY_PS1_PATH cache variable. * [CrashReporting] Drop the installer /CRASHTEST hook, tidy the log sender Remove the /CRASHTEST switch and the matching COCKATRICE_SENTRY_DSN environment override on the installer abort path. With no deliberate in-installer trigger left, a package built without a baked DSN must never offer to post a user's log to an endpoint chosen by whatever happens to be in the environment, so the prompt is now gated on CPACK_SENTRY_ENABLED alone. This also retires the comment that claimed /S /R /CRASHTEST reaches the prompt: the switch was evaluated before /R set ReinstallMode, so that form never showed it. Alongside: correct the CPACK_SENTRY_RELEASE comment (PROJECT_VERSION keeps its "-