216 commits

Author	SHA1	Message	Date
Lukas Brübach	1893dbeeb0	[Tests] Drop the empty icon baseline ... The legacy icon set is fully paid down, so the ratchet had nothing left to ratchet: BaselineIsWellFormed required the baseline to be empty, which made the baseline file, the stale-entry diff and the entry-format regex unreachable. The rules are enforced directly now. - AssetsFollowTheRules fails on any violation of collectViolations(). - tests/icon_assets/icon-baseline.txt is gone with its reader.	2026-11-04 18:29:34 +01:00
Lukas Brübach	8a933ea8f6	[Icons] Add an icon asset ratchet test ... The icon rules were enforced by review only, so the next Inkscape export could reintroduce a baked fill or a physical size without anyone noticing. Add a test that rejects non-24 viewBoxes, physical sizes, editor metadata, hardcoded paints, missing tokens, color-coded filenames, and rasters. Keep the legacy baseline empty so the ratchet can only tighten.	2026-11-04 18:29:34 +01:00
Lukas Brübach	ba05e2d094	[Tests] Keep the icon render test QApplication alive until exit ... Creating it as a function-local static tears it down before Qt's library statics at exit, and libQt6Network then runs QObject destructors against freed memory — a SEGV the sanitizer leg reports after the tests have already passed. Leak the application instead, the usual pattern for a test binary.	2026-11-04 18:29:28 +01:00
Lukas Brübach	fa5133071d	[Icons] Migrate UI glyphs to palette-token SVGs ... Runtime inversion is not a theming system: it rotates hue, breaks duotone art and mishandles alpha, and a QPixmap snapshot cannot follow a palette change. Redraw the top-level UI glyph set on a 24x24 grid with `currentColor` as the paint token, drop Inkscape/Sodipodi cruft, and delete the icons whose metaphor was duplicated (cogwheel/gear -> settings, scale_balanced -> scales, update.png -> reload). Load the glyphs through `loadThemedIcon` so the engine resolves the token from the palette on every request, and let `themePixmap` use the same resolution for one-shot pixmaps. `loadColorAdjustedPixmap` is gone; every caller now keeps a QIcon, which `SettingsButtonWidget::setButtonIcon` accepts instead of a QPixmap. Content art (mana, counters, countries) keeps its authored colors. Add a render test covering aspect ratio, palette flips, icon modes and high-DPI pixmaps.	2026-11-04 18:29:28 +01:00
Lukas Brübach	8e367f37ec	[Icons] Add SVG currentColor sentinel resolver ... Qt's SVG renderer has no CSS cascade, so a `currentColor` fill has to be substituted before the bytes reach QSvgRenderer. - Match and replace on bytes: both the sentinel and the replacement are ASCII, so the document never goes through a UTF-16 round trip, and files that are not valid UTF-8 survive unchanged. - Document that the match is a plain case-insensitive substring, so an identifier that merely starts with the sentinel gets rewritten too.	2026-11-04 18:29:06 +01:00
Bryce!	0de92c06f8	[Spoilers] Preserve spoiler files through update and reload failures (#7707) ... * Preserve spoiler files and serialize update reloads * Surface spoiler removal reload failures and allow disabled-state retry * Address review feedback on spoiler update lifecycle Only remove the spoiler file when the user unchecks the setting; opening Settings no longer queues an unrequested reload for a leftover file. Remove the spoilersUpdatedSuccessfully signal, which had no receiver. * Fix spoiler settings state after reopen and while disabled Reopen Settings while an application-owned update is running: the new page now observes that updater, so the button reads Updating... and re-enables on completion instead of offering a second update that cancels the first. Observation does not take ownership: only updaters the page creates are retired with deleteLater, because the launch check is stack-allocated. When spoilers are off, the hint now describes what Reload Card Database actually does, stays readable, and wraps instead of clipping. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-04 18:24:19 +01:00
Bryce!	3a656c9c4a	Preserve installed card databases when Oracle refresh fails (#7739) ... * Preserve installed card databases when Oracle refresh fails * Fix the Oracle refresh verdict and cover the parser write path A background refresh that already installed cards.xml exited 1 when the token save afterwards failed, so the client kept its old in-memory database and reported the update as failed. Return the same verdict main() already computes: a reject after the database was written is a successful update. Cover the writer Oracle actually uses: add a short-write death test for CockatriceXml4Parser::saveToFile, which reverted to QFile would truncate an installed database without any test noticing. Route a failed background token download through reject() instead of a modal box on a hidden wizard, which left the client waiting with no timeout and blocked later updates. Test the first non-space byte in place rather than copying the payload, and report an import refusal with the importer's own message. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-04 17:33:49 +01:00
Bryce!	e558c0dffa	[Banner] Keep horizontal user banner crops inside the artwork (#7742)	2026-11-04 17:08:54 +01:00
BruebachL	5ea16c805f	[Server/Client/Protocol] Shuffle or reorder player seats before a game starts (#7398) ... * [Protocol] Add shuffle players option and player order command/event Adds a 'Shuffle players at start' option to game creation plus a local player id on the game state so clients can reconcile seat changes, and a SET_PLAYER_ORDER command (1206) with a PLAYER_ORDER_CHANGED event (2023) so the game host can set or randomize the seating order. Took 6 minutes * [Server] Shuffle and reorder player seats before a game starts Auto-shuffles the seats at game start when enabled (now for games with two or more players instead of three or more) and handles the new SET_PLAYER_ORDER command so the game host or a moderator can reorder or randomize the seats before the game starts. Re-seating re-keys the player ids while keeping the per-user game/seat routing and persistent player records in sync, then broadcasts the new order to everyone. * [Client] Add shuffle players option and turn order dialog Adds the 'Shuffle players at start' checkbox to game creation and a host-only 'Set turn order...' action that opens a dialog to reorder or randomize the seats before the game starts. Seating changes are shown in the game's message log. * [Client] Move the turn order dialog strings into retranslateUi Follows the DlgInviteToGame convention so the dialog can be retranslated instead of freezing its strings at construction time. * [Server] Keep player ids stable across seat shuffles * [Client] Lay out table and turn order dialog in seat order * [Server] Send game-joined player list in seat order * [Client] Reword the shuffle option to match its behaviour * [Client] Address the seat-order review comments - Give the game setup checkboxes a row of their own so they no longer collide in the create-game dialog - Rename DlgTurnOrder::randomize() to isRandomizeRequested() - Document what the ids in GameState::seatOrder mean - Keep "Set turn order..." in sync with the game: hidden for non-hosts and once the game has started, re-shown when the game ends or hosting moves to the local player (new GameState::hostChanged signal) * [Client] Do not preview a randomized turn order Randomizing is a request to the server, not a local shuffle. Shuffling the list locally meant the dialog showed an order the server then discarded, and let the host re-open it until a favourable order appeared. The dialog now switches to a mode that says the server will randomize and toggles back to the manual list on request. * [Client] Let moderators set the turn order The server accepts SET_PLAYER_ORDER from the host or a moderator, but the client hid the action from everyone except the host, making the moderator branch unreachable. Visibility and the handler now share canSetTurnOrder(). * [Server] Rely on the player order event instead of a full state resend cmdSetPlayerOrder re-sent the whole game state after reseating, so every client reloaded its deck view mid-lobby for no reason. The new seating reaches clients through the Event_PlayerOrderChanged that reseatPlayers broadcasts. * [Client] Show random turn order in the game selector The restrictions column listed "open decklists" but not the new seating option, so a joining player had no way to see that seats will be randomized each game. Game info already shows the disabled checkbox. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-04 15:44:50 +01:00
Bryce!	ca2301bc96	Fix deck export and mana group ordering (#7729)	2026-11-04 15:43:38 +01:00
BruebachL	d8700b2947	[Fonts] Add AppFont roles and FontConfig fonts.toml round-trip (#7534) ... Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-04 07:57:52 +01:00
Bryce!	94f7de42f2	Fix user card games for offline users and stale requests (#7738)	2026-11-04 05:29:51 +01:00
Bryce!	dc4aa19d23	Import Moxfield signature spells into the deck [skip ci] (#7751) ... Co-authored-by: Codex <codex@openai.com>	2026-11-03 10:40:28 +01:00
Jurriaan	bc09ec6afe	[Client] Make DeckStateManager::decrementCard remove tokens from the token zone if the token is not in the given zone (#7588) ... * Fixed decrementCard for tokens not using the token zone * Removed dead function actDecrementCard Updated documentation * Fixed no-op, added documentation * Removed stray blank line * Allow finding card nodes even if no database entry exists for the given card * Added DeckStateManager tests * Added tokens on mainboard test * Added if checks and sorted sources Added if checks * Library include fix * Renamed zone to targetZone * Revert "Allow finding card nodes even if no database entry exists for the given card" This reverts commit 88578cd0d9. * Combined loader_local_matching_test and deck_state_manager_test settings sources * Added settings sandbox, refactor * Format * Renamed return variable * Flattened if statement * Style fix	2026-11-03 10:37:17 +01:00
sentry[bot]	9f30892dff	[Client] Fix crash when local game commands are sent after shutdown (#7714) ... * [Client] Fix crash when local game commands are sent after shutdown * [Network] Fix formatting in local client debug log * [CI] Keep the vcpkg submodule pointer on the manifest baseline vcpkg.json asks for builtin-baseline 9eec002 (vcpkg 2026.10.16), but the submodule gitlink was still at 9e593bb. The runners check submodules out shallowly, so vcpkg-tool cannot read versions/baseline.json out of the baseline commit and every vcpkg leg (Windows and all macOS) fails in CMake configure with: failed to `git show` versions/baseline.json git phrases that as "path ... exists on disk, but not in <sha>" even when the commit is simply absent from the shallow clone, so it reads like a tooling or network failure rather than a mismatched pointer. Point the submodule back at 9eec002, and add a cheap job that fails when the gitlink and builtin-baseline disagree, before the five build legs pay for it. --------- Co-authored-by: sentry[bot] <39604003+sentry[bot]@users.noreply.github.com> Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-02 13:28:23 +01:00
RickyRister	62e590d300	[ServerGame] Refactor out spectator/judge bools into RoleInfo (#7761)	2026-11-02 00:37:05 -07:00
BruebachL	a1a8a59f4a	[VisualDatabase] Draw the card grid with a model/view (#7701) ... * [VisualDatabase] Draw the card grid with a model/view instead of a widget per card The visual card database built one CardInfoPictureWithTextOverlayWidget per card, 100 per page, and tore them down on every search, resize and scroll page. It also looked each card up again by name from the model row, kept a shadow QList<ExactCard>, and carried paging state plus a 5 s timer that only existed to defer the first page. Replace that with a real view on a flattened model: - CardDatabaseDisplayModel gains CardInfoRole and cardInfoAt(), so a view can get the CardInfoPtr behind a proxy row without a name round trip. - New CardDatabaseVisualDisplayModel (libcockatrice_models) flattens the filtered and sorted proxy into one entry per displayed card picture: the preferred printing normally, every printing of the selected sets while set filters are active. Proxy changes rebuild it on a 30 ms coalescing timer. - New CardDatabaseVisualView (QListView IconMode) paints only visible rows through the new CardPictureDelegate, keeps the old interaction contract (left/right/middle press -> cardClicked, hover -> cardHovered, Ctrl+wheel resize, no selection, no focus stealing, scroll to top on a new result) and repaints when CardPictureLoader reports an async image. - CardSizeWidget::enableCtrlScrollResize() also watches scroll-area viewports, which is what makes Ctrl+wheel work over a QListView. - VisualDatabaseDisplayWidget drops the paging, the shadow list, the dead members and the 5 s timer. Behaviour-preserving: no gameplay change, no new user-facing strings. * [VisualDatabase] Fix the card grid wheel step QListView hands the delegate's item height to the scroll bar as its single step under ScrollPerPixel, so QAbstractSlider moved wheelScrollLines() whole card rows per notch. Handle the wheel in the view instead: one line is an eighth of the grid cell, so a notch moves about a third of a card row at any card size and still follows the OS line count. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-02 08:35:58 +01:00
BruebachL	3cf8bdb00e	[Tests] Add a headless game logic regression suite (#7702) ... * [Tests] Add headless game logic regression suite Cover CardState, CardZoneLogic and the Command_MoveCard protobufs PlayerActions emits, against libcockatrice_game with a recording AbstractClient and no graphics. The old game_smoke_test cases are migrated into the new suite. * [Tests] Cover game event application through GameEventHandler Feeds GameEventContainers through the real dispatcher and asserts the resulting zone and card state: move (including face down), the set-card-attr attributes plus the all-cards and does-not-untap branches, card counters, attach/detach, zone reveal properties, and drawing named cards from the deck. * [Tests] Replay a synthetic game state through the dispatcher Builds a GameReplay in code, feeds its containers through the same GameEventHandler path the client uses, and asserts the materialized players, zones, cards and attachments plus event ordering for move, attributes and counters. A small headless zone view stands in for the graphics side where processPlayerInfo asks for card items. * [Tests] Cover card flip, destroy, token and turn events Adds the remaining state-mutating event handlers: flip (face down keeps the ref, face up adopts the printing), destroy (removes the card and un-attaches its attached cards), create token (database P/T unless provided or face down) and player properties changed under the concede/unconcede contexts, plus active player and phase. * [Tests] Cover player lifecycle and counter events Adds the player-level events that mutate session state: leave removes the player from the manager, kicked emits gameClosed, game closed flips the closed flag and clears the started flag, host changed moves the host role to another player, and create/set/delete update the player counters with unknown ids ignored. * [Tests] Drive the zone view request through addCard The test emitted createCardRequested itself and then asserted it was emitted. Registering a ZoneViewZoneLogic on the zone and adding a card exercises the real prepareAddCard -> signal path, so the assertion now fails if addCard stops asking the graphics side to build the card. * [Tests] Skip the card database when listing tests gtest_discover_tests runs the binary with --gtest_list_tests during the build. That process built the card database and replaced its default path provider, leaving allocations that the sanitizer leg's default LeakSanitizer check reports at exit, so discovery failed on Ubuntu 26.04. No test runs in list mode, so return before the database setup. * [Tests] Fix fixture comment nit Wraps the RecordingClient::sent member name in backticks in the comment. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-02 07:35:41 +01:00
BruebachL	5134e72eb4	[Settings] Store server profiles with QSettings arrays (#7629) ... The server list used hand-numbered saveName%1/server%1 keys and an O(n) shift loop to delete entries. Store the profiles with beginReadArray and beginWriteArray instead, and migrate the legacy keys once in the constructor so existing profiles and saved passwords survive. SettingsManager::batchWrite no longer disables atomic sync, so a batch is still written through QSettings' atomic save. Add tests for add, update, remove, password updates and the legacy migration. Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-02 07:30:29 +01:00
BruebachL	b37c8b828f	[Utility] Budget scrypt's XY array in the derivation memory limit (#7712) ... deriveKey passes maxmem = 128nr + 128rp + 4096 to EVP_PBE_scrypt, but scrypt also allocates XY at 256r bytes. costParamsAreSane accepts r up to 32, so any server asking for r >= 17 made derivation fail and the client report a login abort it could never recover from. Cover the XY array and add a regression test at the accepted-range boundary. Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-02 07:29:15 +01:00
BruebachL	ce543896f5	[Tests] Use QtTest for model contracts and signal spies (#7630) ... Qt6::Test was linked into every test but never used. Add a small RAII QAbstractItemModelTester helper and apply it to the deck list model, the card database model and display proxy, and the sets model. Add QSignalSpy coverage for the card-added and download-settings signals, and assert explicitly that a card row is a leaf with no child columns (the tester does not check that). The contract checker immediately found four model bugs, fixed here: - DeckListModel allowed children below non-zero columns. - CardDatabaseModel reported every card as a child of every card row. - CardDatabaseModel reported six columns for a valid (leaf) card index. - CardDatabaseDisplayModel claimed more rows were fetchable while rowCount already exposed them all, so fetchMore() inserted duplicates. Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-02 07:27:43 +01:00
BruebachL	50a2ae9cc8	[Server/Client] Enforce and complete the forced password change at login (#7680) ... checkUserPassword ignored force_password_change on the challenge-response path, so a reset password stayed valid. Returning PasswordChangeRequired was not enough on its own: the session was dropped and the client told the user to ask the administrator for the temporary password they had just used. The pending session now keeps its identity without registering a user session or announcing a join, may only ping and change its password, and the client completes the change in-session before reconnecting. Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-11-02 07:01:29 +01:00
BruebachL	00151380e3	[CardDB] Break the CardInfo <-> CardSet cycle on reload (#7645) ... * [CardDB] Drop the CardInfo self pointer The weak self pointer had to be assigned right after every construction, and a missed assignment silently stopped change notifications. CardInfo is always owned by a QSharedPointer, so it now derives from QEnableSharedFromThis<CardInfo> and asks for sharedFromThis() instead: nothing to initialize, and the CardSet cycle stays broken. CardSet::newInstance() also loses the commented-out self-pointer assignment left over from the same design. * [CardDB] Pin the cardInfoChanged contract without the self pointer Dropping CardInfo's self pointer moved change notifications onto QEnableSharedFromThis, so the signal now rests on the card being owned by a QSharedPointer. Two tests pin that down: - A property change on a card owned by a CardDatabase still emits cardInfoChanged() carrying that card, which is what the card database views and models listen to. - clone() starts out unowned, so a change to the clone is reported as the clone's and must not reach the original's observers. * [CardDB] Treat an unowned CardInfo as a bug, not a no-op Every CardInfo is built inside a QSharedPointer (both factory overloads and clone()), and no constructor notifies anyone, so sharedFromThis() is always available by the time addToSet() or a setter runs. Assert that and warn in a released build, instead of silently skipping the notification or the set append. The copy constructor keeps its explicit QEnableSharedFromThis initializer: GCC's -Wextra warns about the uninitialized base in every translation unit that copies a CardInfo otherwise. A comment there records that the clone deliberately starts unowned. * [CardDB] Break the CardInfo <-> CardSet cycle on reload CardSet is a QList<CardInfoPtr> and CardInfo::addToSet() appends the card to its sets, while a card keeps a CardSetPtr per printing in setsToPrintings. The sets therefore hold strong references back to the cards, and the previous database snapshot stayed alive after a reload, so RSS grew by roughly a second database on every reload. Empty the sets before the cards are dropped: CardDatabase::clear() and swapInDatabaseData() clear the sets before replacing the containers, and removeCard() detaches the card from its sets, so nothing keeps a card alive on behalf of a set once the database lets go of it. Two tests pin the behaviour: a reload and a clear() must drop the last strong reference to a card from the previous snapshot. * [CardDB] Keep the set detach in one place clear() and swapInDatabaseData() both emptied every set before dropping the cards, so share that as detachSetMembership() instead of two copies that have to stay in sync. removeCard() now detaches through the first printing of each set directly, the same way CardInfo::newInstance() appends, instead of an inner loop that always breaks after the first item. Because clear() empties the sets up front, its per-card detach was walking every card's printings to remove it from sets that were already empty. Split removeCard() into the per-card detach and unindexCard(), and let clear() call only the latter. * [CardDB] Test that removeCard detaches the card from its set removeCard() is the one teardown path a caller can trigger for a single card (the token editor uses it), and it now detaches the card from its sets as well. Without that detach the set's strong CardInfoPtr keeps the removed card alive forever, which is exactly what the reload and clear tests pin for the whole-snapshot paths. The test loads the fixture, removes Cat, and requires the card to be gone from the CAT set and to be destroyed once the test drops its own reference. * [Servatrice] Drop the settings cache from the logger #7602 removed #include "settingscache.h" from server_logger.cpp but left isLoggingEnabled() reading settingsCache, so master no longer compiles: the servatrice target fails on the undeclared global and server_logger_test, which compiles the same file without main.cpp, fails to link it. The same commit made reloadConfig() the single place that stores writelog and the log filters, precisely so logging threads stop reading the global QSettings. isLoggingEnabled() now mirrors logMessage() and reads those members instead: the console is a sink of its own, and the logfile needs an open file plus writelog. The mutex is mutable so the const query can take it. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-31 19:37:15 +01:00
BruebachL	825aba953a	[Servatrice] Fail loudly when startup initialization fails (#7602) ... * [Servatrice] Exit non-zero when server initialization fails initServer() reported every startup failure through a zero exit code: the database could not be opened, the server printed nothing of its own, and the process looked like a clean shutdown. Log the two refusal reasons at critical level and return 1 so supervisors and the Docker exit status can tell the difference. * [Servatrice] Keep console logging independent of the logfile logMessage() returned before the console echo unless a logfile was open and server/writelog was true, so --log-to-console produced nothing when server/logfile was empty or could not be opened. That is how a refused database connection could end in a silent exit. Print to the console before the logfile gates, drop the duplicate echo from flushBuffer(), and read writelog/logfilters once in main instead of calling the non-thread-safe global QSettings from every logging thread. * [Servatrice] Uninstall the log handler before tearing down globals The handler reaches into the global logger and settingsCache. Leaving it installed while main deletes those objects turns any Qt warning emitted during QCoreApplication/QFactoryLoader teardown into a use-after-free; with QT_DEBUG_PLUGINS=1 that warning exists, and Servatrice died with SIGSEGV in QSettings::value() on the way out. * [Servatrice] Buffer console logs on the logger thread Address review feedback on the startup failure change: - Append console lines to a buffer drained by the logger thread, so a stalled stdout reader cannot block game threads. - Report islServer->errorString() instead of the literal call text. - Let the constructor reuse reloadConfig() instead of parsing the filter list twice. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-31 19:02:49 +01:00
BruebachL	52d3978f84	[Build] Let AUTOMOC process library headers (#7638) ... The 13 libraries using qt6_wrap_cpp(MOC_SOURCES ${HEADERS}) listed their\nheaders twice and depended on a generated source list. Add the headers to the\ntarget sources directly and delete the qt6_wrap_cpp call; AUTOMOC scans them\nand generates the same moc output.\n\nThe change exposed that server_card_counter_test never defined the rng global\nthat the server remote library references; every sibling server test does.\nGive the test the same definition so the target links.\n\nVerification: full client, Oracle, server and test build passed. Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-31 18:49:05 +01:00
BruebachL	345b0e8ba9	[Refactor] Move game logic into libcockatrice_game (#7437) ... * [Refactor] Move game logic into libcockatrice_game * [Refactor] Include card_state.h in player_actions.h * [Game] Inject settings providers into libcockatrice_game, drop Widgets * [Game] Follow base rename to createCardRequested after rebase * [Game] Drop unused CardItem forward declaration * [Game] Fix includes after moving game logic into libcockatrice_game --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-31 18:44:15 +01:00
BruebachL	47c90cda10	[Refactor] Give graphics zones a card item cache and own card placement (#7427) ... Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-31 18:44:12 +01:00
sentry[bot]	bbc7e462ff	[Installer] Graceful close and a way out of a stuck update (#7687) ... * [Installer] Stop calling the payload free while an executable is locked The guard answered "nothing is holding the payload" from two lookups that can both miss a running instance: Restart Manager does not reliably report a program's own image or the modules it mapped, and the image path of a process running under another account cannot be read from the probe at all. A user log showed exactly that: the first guard passed while cockatrice.exe was running from the directory, the old uninstaller silently skipped the file it could not delete, and the update aborted half an uninstall later with "cancelled by user" after a close attempt the running window ignored. Ask the file itself. A running image is opened without write sharing, so the write open replacing it needs is refused with ERROR_SHARING_VIOLATION for as long as the program runs, while a file that is merely being read still accepts it. When neither lookup named anybody and a payload executable refuses that open, the probe answers that the file is in use and that the program holding it could not be identified - the guard's "could not confirm" dialog, whose Retry works once the user has closed it - instead of passing the payload as free. Only the payload executables are tested, not every file the purge removes: a locked executable proves a program is running and a mapped DLL belongs to a running executable, while a locked DLL or qt.conf can be the asking process itself, which is the loop the guard already had. The open is retried once after 300 ms, and only for the files that refused, so a scanner holding a file for a moment cannot stop an install for nothing. The Windows probe check now covers that answer: a payload executable held by the asking process, so that it cannot be named, has to come back as in use rather than as free. Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de> * [Installer] Close running clients over a local socket The installer could only ask a running client to close with WM_CLOSE, which an instance hidden in the notification area or sitting on its own confirmation cannot answer usefully. Give every client a per-process local socket and have the holder probe ask it there first, falling back to WM_CLOSE for clients from before the socket. * [Installer] Offer to check again or update by hand when files stay locked Replace the two-answer guard prompt with force close, close by hand and check again, or cancel. Retry the automatic close while the set of holders changes, and point a cancelled run at the latest release so a user who does not want anything killed has a way forward that is not cancelling the update again. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-31 15:32:59 +01:00
sentry[bot]	0369aaffa9	[Installer] Name only the payload programs installed in the directory (#7684) ... The running-instance lookup named every process whose image path began with the install directory, subfolders included. Installing into a folder that holds another application in a subfolder therefore reported that application as holding the previous version, and nothing the guard offers could clear it: the close asks cockatrice.exe only, and the end answer refuses anything that is not a payload program. Both answers were no-ops, so the update could only abort and tell the user to close a program that was never touching the files. Name only the programs the payload installs, and only when the image lies in the install directory itself. A program below the directory that really does hold a payload file is still named by the Restart Manager lookup, which is the lookup for that question. The Windows probe check now covers a payload-named program in a subfolder and a program the payload does not install; both have to come back as nothing. Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-30 13:16:50 +01:00
BruebachL	675e1f7447	[Translation] Name languages without reading the translation files (#7662) ... * [Translation] Name languages without reading the translation files The language selector derived every label from the language's own .qm file: it loaded the file and translated the literal "English" in the "i18n" context. lupdate could never extract that literal from the callers, which pass the DEFAULT_LANG_NAME macro, so a dummy QT_TRANSLATE_NOOP in cache_settings.cpp existed to give it something to extract. #7050 deleted that dummy, the next lupdate push dropped the string from Transifex, and the following pull rewrote the .ts files without it. lrelease only emits translated messages, so 22 of 25 .qm files returned an empty name. The names are data, not translations: a selector has to be readable before any translation is loaded, and a label must not depend on the translation of the language it names. TranslationLoader::languageDisplayName now holds them next to availableLanguages, falling back to Qt's native language name and then to the code, so a language added later still gets a label. Also select the configured language by item data instead of by matching the translated "English" string, which pointed at the wrong row whenever the lookup came back empty, and restore that English entry's own name: en_US is "English (United States)" so it can no longer be confused with the untranslated default. Oracle shared the code path, so its wizard language page is fixed too. * [Translation] Name the default language selection once Review follow-up: log the unresolvable language through the page's own category, and move the en_US entry both selectors fall back to for an unset setting into TranslationLoader::defaultLanguageCode. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-26 02:39:26 +01:00
BruebachL	5fbc636f93	[Models] Use QCollator for natural, cached sorting (#7633) ... * [Models] Use QCollator for natural, cached sorting QString::localeAwareCompare rebuilds its collation data for every comparison, and it compares numbers as text. Keep one QCollator per model, enable numeric mode, and use it for the card name, set and user name sorts. Add a test that sorts "Forest 1", "Forest 2", "Forest 10" through DeckListSortFilterProxyModel and checks the natural order. * [Models] Sort naturally under the C locale too Numeric mode only takes effect through a real locale: under the C locale, which headless CI processes run in, QCollator silently falls back to string order and sorts "Forest 10" before "Forest 2". That made collator_sort_test fail on every Linux leg while passing locally. Add makeNaturalSortCollator(), which enables numeric mode and pins English only when the process locale is the degenerate one, and build the models' collators through it. Users with a real locale keep their own collation. * [Models] Fix compile of natural sorting (QStringView mismatch) * nit: initialize collators in member initializer lists * nit: namespace makeNaturalSortCollator under NaturalSort --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-25 01:01:04 +02:00
BruebachL	518c7e6125	[Network] Add a configured QNetworkAccessManager factory (#7628) ... * [Network] Add a configured QNetworkAccessManager factory Every HTTP caller built its own QNetworkAccessManager, so the system proxy and a transfer timeout were easy to omit. Add NetworkManagerFactory::create() as the single place that applies the process-wide proxy configuration and a transfer timeout, and use it at all 14 construction sites. Per-site settings such as disk caches and manual redirect policies stay with their callers. * [Tests] Link libcockatrice_network into the tests that compile its headers release_channel.cpp and card_picture_loader_worker.cpp now include libcockatrice/network/network_manager.h, but the test targets that compile them only linked libcockatrice_settings, so the include directory never reached them and every platform failed with "file not found": loader_local_matching_test and picture_loader_benchmark_test. libcockatrice_network is an INTERFACE library, so linking it costs these targets nothing but hands over the include path. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-25 00:33:43 +02:00
BruebachL	4028606bd9	[CardDB] Clear CardInfo relations before deferred deletion (#7615) ... removeCard() and swapInDatabaseData() schedule CardRelation deletion with deleteLater(), but the owning CardInfo could outlive them through an external shared pointer and kept returning the freed pointers from getAllRelatedCards(). Add CardInfo::clearDirectRelations() and call it after scheduling deletion so the UI can no longer dereference dangling relations. Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-24 10:28:24 +02:00
BruebachL	bbc6003f0a	[CrashReporting] Name the files and processes behind a failed installer purge (#7590) ... * [CrashReporting] Log which files and processes blocked the installer purge A purge that fails aborts the upgrade with a message that can only guess at the cause, and the log only listed which DLLs survived. Record what Windows reports for each survivor and which processes still have the install directory mapped, then repeat the holder list in the abort message. The diagnostics are additive and run only on the abort path: they open each survivor for DELETE access and close it again without deleting, so a failed upgrade can never end up half-removed. The purge itself is unchanged. * [CrashReporting] Name the holders of a failed purge with Restart Manager The process-and-module scan could only name a holder whose image lives in the install directory or that loaded one of its DLLs, so the case this diagnostic exists for - an antivirus scanner holding a plain handle - was reported as "no holder named, only a code". Reaching the Restart Manager from the NSIS script would have meant marshalling RM_PROCESS_INFO through System::Call as a table of hand-computed offsets, which cannot be checked without a Windows toolchain, and that is why it was dropped when this diagnostics was written. Move the call into tools/installer-holder-probe: a small executable the same Windows build compiles, linking nothing from this project so it still starts when the installed runtime is what the purge could not replace. It rides inside the installer and is extracted to $PLUGINSDIR only when the purge has already failed, so the successful path is unchanged and nothing new is installed. .ci/compile.sh runs it against a file held with FileShare.None on the Windows runners, so the lookup is exercised on real Windows instead of only compiled. * [CrashReporting] Fix the holder probe check and cap the holder line The probe block ran after the package steps, where the working directory is already the build dir, so its $BUILD_DIR-prefixed paths named build/build and the PowerShell file could not be written. Move it ahead of install/package, where the build dir is still the cwd, and name both files relative to it. Guard it on MAKE_NO_CLIENT, since the probe is only built with the client. Cap the rendered holder line at HOLDER_LINE_CHAR_MAX: the count limit alone does not keep long service names inside the NSIS string that also carries the abort text and the log path. Whole entries are kept and the rest counted, with the first holder always shown. Also run cmake-format on the probe's CMakeLists so the lint job accepts it. * [CrashReporting] Report a read-only purge survivor and bound the probe check --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-24 00:46:54 +02:00
BruebachL	ad4320cd7b	[CardDB] Stream card properties from blobs (#7608) ... * [CardDB] Stream card properties from blobs Parsed and cached cards used to keep a materialized QHash<QString, QString> for their properties: LazyPropertiesHash only deferred building that hash until the first query, and the cache writer re-serialized it on save. - LazyPropertiesHash answers value(), contains(), size(), isEmpty() and keys() straight out of the serialized blob, so a property query no longer builds a hash. - serialize() is public, and the cache writer passes the stored blob through instead of serializing a hash it just rebuilt. - CardInfo and PrintingInfo expose getPropertiesBlob() and getPropertyCount(); PrintingInfo::operator== compares key by key without materializing either side. - The XML parsers build cards via newInstanceFromPropertiesBlob(), which derives the simple name and the alternate names that the parser used to keep next to the hash. The reload cycle fix stays out of this change; it depends on changing the CardInfo self pointer and is reviewed separately. * [CardDB] Address review comments on streaming properties * [CardDB] Address final review nits on property streaming Move BlobReader and the blob accessors into LazyPropertiesHash as private members so the parsing helper is declared with the class instead of hiding in the translation unit. Add LazyPropertiesHash::serializedFrom() and use it where a PrintingInfo is built from a fresh property hash, replacing the nested LazyPropertiesHash(LazyPropertiesHash::serialize(props)) construction. The pre-materialized QHash constructor stays: CardInfo's non-blob factory still uses it for the oracle import path, and the tests cover it directly. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-24 00:17:31 +02:00
BruebachL	0fe4fd0d8c	[CardDB] Guard guessCard and findPrintingWithId against missing cards (#7592) ... * [CardDB] Guard guessCard and findPrintingWithId against missing cards guessCard() dereferenced a null CardInfoPtr through findPrintingWithId() when the name was unknown and a providerId was given. Return an empty ExactCard instead, and make findPrintingWithId() null-safe for every caller. Add regression tests covering both paths. * [CardDB] Rely on PrintingInfo's default null set in findPrintingWithId The explicit nullptr was redundant: the constructor already defaults the set to null. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-20 21:33:56 +02:00
BruebachL	9980c90cea	[Filter] Allow spaces around the colon in search queries (#7529) ... * [Filter] Allow spaces around the colon in search queries * [Filter] Keep whitespace out of the color-identity operator Allowing `ws?` between `color` and the identity marker made `c i: g` parse as identity green, changing the meaning of text that previously read as a generic `c` plus a field query on `i`. Whitespace belongs around the separator, not inside an operator token: `ci : g` and `ci: g` stay valid, `c i: g` keeps its old parse. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-20 21:20:10 +02:00
BruebachL	9f59f5c495	[CrashReporting] Breakpad capture, session detection and symbol upload (#7523) ... * [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 <db>/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/<target>/Release, which is the Visual Studio generator's layout. The Windows leg uses Ninja, which is single-config: linker PDBs land in build/<target>/, 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 <run> -n <name>-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 <Bruebach.Lukas@bdosecurity.de>	2026-10-19 14:57:04 +02:00
BruebachL	ad0fd2d4b0	[CrashReporting] Sentry send path with consent gating (#7522) ... * [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 <database>/cache and AlwaysSend's transport-dumped files in <database>/<run>.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 <Bruebach.Lukas@bdosecurity.de>	2026-10-19 14:57:03 +02:00
BruebachL	6385f3fab3	[CrashReporting] Mock crash-report dialog and reporting preference (#7521) ... * [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 <temp>/../.., 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 <Bruebach.Lukas@bdosecurity.de>	2026-10-19 14:57:03 +02:00
Zach H	ea6af0048c	[Server] Parse the rightmost address from forwarded header lists (#7586) ... X-Forwarded-For carries a comma-separated list, and constructing a QHostAddress from the raw header value returns a null address for it, so a configured X-Forwarded-For or a multi-hop header never resolved and websocket clients stayed on the proxy's address. - Add parseForwardedAddress, returning the rightmost usable entry of a forwarded header list - Use it for the websocket handshake instead of parsing the raw value - Exempt loopback from per-address auth rate limits so a same-host proxy with a customized trusted_sources list cannot lock every user out - Add forwarded_address_test Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-18 16:53:55 -04:00
Zach H	46cdbac830	[Servatrice] Match trusted_sources by address, not substring (#7582) ... trusted_sources was checked with QString::contains. A dual-stack listener reports a local proxy as ::ffff:127.0.0.1, which never matches the default "127.0.0.1,::1". Behind nginx the forwarded client IP header was then ignored, every websocket user shared the proxy's address, and logins failed with "too many concurrent connections" once max_users_per_address was reached. Add isTrustedSource(), which parses each entry as a QHostAddress and compares exact addresses with IPv4-mapped addresses normalized, and use it at all four call sites. The ban command also read server/trusted_sources instead of the documented security/trusted_sources; it now uses the same getter. Entries that are not IP addresses are logged at startup since they no longer match anything. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>	2026-10-18 16:50:04 -04:00
BruebachL	a139084ad6	[Security] Add challenge-response auth with scrypt verifiers and stop storing plaintext passwords (#7193) ... * [Security] Add challenge-response auth with scrypt verifiers and stop storing plaintext passwords Challenge-response authentication: the client derives a scrypt verifier (RFC 7914, EVP_PBE_scrypt, N=32768, r=8, p=1) and authenticates with HMAC-SHA256(key, nonce), so neither the password nor its hash is transmitted. The stored format becomes "$scrypt$<n>$<r>$<p>$<salt>$<verifier>" and Response_PasswordSalt now carries the cost parameters. Strict servers only accept scrypt verifiers; legacy accounts are migrated after a successful login. Fix #344 for challenge-response servers: a saved profile stores the derived verifier under the password key instead of the plaintext password. The connect dialog loads it without revealing it, autoconnect passes it through, the change-password dialog no longer prefills the old password field with it, and the client only persists the verifier when "Save password" is checked. Took 3 minutes Took 1 minute Took 10 seconds Took 7 minutes * [Security] Harden credential format acceptance and challenge nonce validation * [Security] Refuse KDF downgrade; trim profile name and warn on silent credential drop * [Security] Add SupportsAll ServerOptions enumerator to fix enum validation assert The server advertised both SupportsPasswordHash (1) and SupportsChallengeResponseAuth (2) via a bitwise-OR that produced the value 3, which no enumerator declared. Protobuf's proto2 enum validation rejects the value in _internal_set_server_options, crashing servatrice on the debug assertion. Add a SupportsAll = 3 enumerator with an identical wire value so the bitmask reads unchanged while the value is now a documented, valid member of the enum. * [Security] Normalize username when issuing challenge nonce cmdRequestPasswordSalt bound the challenge nonce to the raw, un-simplified username while cmdLogin verifies it against the simplified name. A user who typed leading or trailing whitespace in the username box would be silently rejected during challenge-response login even with the correct password. Simplify at the nonce-issuing site so both commands agree on the normalized name. Also drive-by formatting: collapse the now single-expression serverOptions initializer onto one line. * [Security] Keep an encrypted legacy password backup for old-client logins Migrating an account to scrypt stranded pre-challenge clients, which can only present the legacy salt+hash: after migration the server served them a scrypt salt they cannot use, so the only recovery was a password reset. When security/legacy_backup_key is configured (64 hex chars, openssl rand -hex 32), submitPasswordVerifier now seals the legacy credential with AES-256-GCM under that key before overwriting the row, storing it in password_legacy_backup with the time it was last used. Old clients keep working: cmdRequestPasswordSalt serves the backup's 16-char salt in password_salt so they can derive the legacy hash they were always able to compute, while new clients derive the verifier from the new scrypt_salt field, which always carries the verifier salt. The non-challenge login path accepts the plaintext or legacy hash against the decrypted backup and refreshes its window; a migrated row with no usable backup fails closed. Backups age out so a database dump taken after the window closes never carries a crackable legacy hash: getUsableLegacyBackup purges on first use past security/legacy_backup_ttl_days (default 90, clamped 1..3650), and a six-hourly sweep drops backups idle accounts never touch. changeUserPassword clears the backup since a new credential invalidates it. Without a configured key nothing is sealed or purged, so a temporarily missing key cannot destroy existing backups. The AAD is the simplified user name on both sides, sealing the blob to its row so it cannot be moved between accounts. Sealing failures degrade to the previous lockout behaviour rather than blocking migration. The columns extend the unmerged 0036->0037 migration, so the schema version stays 37. * [Security] Address review: proto compat, plaintext verify, AAD case, strict scope Fix the two blockers and follow-ups from the 2026-10-16 review: - Advertise challenge-response in a new supports_challenge_response field instead of the ServerOptions enum and drop SupportsAll: proto2 enums are closed, so already-released clients parsed server_options = 3 as NoOptions and fell back to plaintext passwords. server_options stays exactly SupportsPasswordHash on the wire; old clients ignore the new field. - Verify a plaintext password directly against a migrated scrypt row with verifyPassword() so email changes (cmdAccountEdit) and old clients sending password keep working without a legacy backup key. Pre-hashed values are still checked against the backup, and its TTL window is refreshed only when the backup is what actually matched. - Use the stored row name as the AES-GCM AAD on both seal and open so a login whose case differs from the stored name (utf8mb4_unicode_ci) still opens its backup. - Require challenge-response in strict mode only for registered SQL accounts (userExists gate), leaving guest/unregistered logins and none/password auth methods unaffected. - Document in servatrice.ini.example that a captured legacy hash can take over a not-yet-migrated account and that strict only constrains registered SQL accounts. Schema version is 38. * [Security] Raise legacy backup TTL default to 180 days Gives operators and slow-to-upgrade users a longer downgrade window for pre-challenge clients before unused encrypted legacy password backups age out. * [Security] Fix servatrice.cpp formatting per clang-format-18 * [Security] Add missing forward declaration for Command_SubmitPasswordVerifier The cmdSubmitPasswordVerifier declaration references the proto-generated type, which otherwise does not name a type in this header and breaks every C++ build. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de> Co-authored-by: BruebachL <bruebachl@cockatrice.example>	2026-10-16 22:52:26 +02:00
BruebachL	ce51e7147d	[Servatrice] Log room channel messages in audit log (#7479) ... * [Servatrice] Log room channel messages and bump schema to 38 * Fixup: show room_channel audit rows and keep migration nullable The log history query returns room_channel rows, but the log tab dropped them. Render them in the room table with an extra Channel column. Also drop the NOT NULL on target_type from the 37->38 migration so migrated schemas match the nullable servatrice.sql definition (and strict-mode ALTERs cannot fail on legacy NULL rows). Update the channel tests to the flag-based privilege model: the admin fixture now carries IsModerator alongside IsAdmin, as the database layer assigns. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-16 22:46:39 +02:00
BruebachL	c99113ab0e	[Client] Tabbed room channels with unread states and collapsible chat (#7478) ... * [Client] Add room channel selector with per-channel chat views * [Client] Tabbed room channels with unread states, collapse, read-only, banners Reworks the room chat channel display: channels move out of the combobox into a tab bar below the message input showing every channel available to the user. - Each tab shows its unread message count and is tinted when it holds unreads or a mention; both colors are user-configurable chat settings (channelUnreadColor, channelMentionColor) with a settings-page section. - Clicking the active tab collapses the chat: message view and input hide and the splitter section minimizes to the tab strip; expanding restores the previous splitter sizes. Viewing a channel clears its unread state. - Channels the user may read but not write (per write_access_level) show with the message input disabled and a read-only placeholder; sends are guarded locally, the server still enforces. - Channel banner_message strings render at the top of their channel view on join, like the room join message on Main. - Replayed channel history no longer inflates unread counts; moderation redaction now applies across all channel views. * Fixup: keep chat tab bar visible when chat is collapsed QSplitter children are collapsible by default, so setSizes({1, 0}) hid the whole chat group box including the channel tab bar. Mark the chat widget non-collapsible so collapsing clamps to its minimum size (title + tab bar) instead. * [Room] Explicitly initialize MAIN_CHANNEL per review * [Room] Address channel tab review follow-ups - gate @all mentions on moderator/admin sender to match ChatView, whole-word matching for own name and highlight words - hide single-channel tab bar so old servers do not show a lone Main tab that collapses chat on misclick - capture target ChatView in sendMessage so errors land in the channel the message was sent from - share ignore/unregistered filtering between RoomSay and RoomChannelSay via isRoomMessageIgnored * [Client] Fix tab_room.cpp formatting per clang-format-18 --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de> Co-authored-by: BruebachL <bruebachl@cockatrice.example>	2026-10-16 22:46:39 +02:00
BruebachL	6c660d6b21	[Installer][Client] Back up user data before old uninstallers can delete it (#7503) ... * [Installer][Client] Back up user data before old uninstallers can delete it * [Installer][Client] Fix stale backup resurrection, batch session backups, extend coverage NSIS: clear stale %TEMP% backup and only arm restore when something was copied, directory-existence checks, back up loose .dec decks, drop downloaded pics but keep pics\CUSTOM. Helper: single session dir per run (ms timestamps), same-basename disambiguation, partial copies still report their location. * [Installer][Client] Fix review findings: throttle backups, NSIS old dir, portable messaging * [Installer][Client] Fix review findings: backup markers, NSIS relocate, size cap * [Installer][Client] Fix review findings: sounds, session coverage, NSIS wording Client: back up data/sounds in portable mode and mention it in the warning; reuse a recent session only when complete and covering the current sources (source manifest), estimate size only for fresh copies, normalize dedup via isInsideDir. Helper: source manifest per session with sessionCoversSources(), clear out-params on all failure paths. NSIS: document defensive portable branch, merge backup over surviving data/ to repair partial deletes, copy (not move) wording in relocate, note .txt exclusion in deck notice. * [Installer][Client] Fix review findings: stale backup, empty reuse, relocate, portable warning NSIS: wipe TEMP backup once per run to avoid resurrecting deleted files, check data dir existence in relocate so dir-only trees are moved. Client: never reuse a recent session for an empty source set, only fall back to a covering session on copy failure, warn fresh portable installs instead of staying silent. * [Installer][Client] Fix review findings: usable session reuse, loose decks, NSIS pics, symlinks * [Installer][Client] Fix portable backup exclusions in offender merge * [Client] Split at-risk data warning into focused helpers Ricky's nit on the 200+ line warnAboutDataPathsInProgramDir: extract collection (portable/installed), backup, message building and dialog into named helpers. No behavior change, all tr() strings untouched. * [Client] Read path settings inside isPortableBackupExcluded * [Client] Make stateless at-risk helpers static * [Client] Extract at-risk data warning out of MainWindow * [Installer][Client] Fix review findings: subdir backup, portable warning, backup API NSIS: - Back up unknown top-level install subdirs (e.g. <installdir>\decks), not just loose deck files, so the 3.1.1 -> new upgrade no longer loses data folders the old RMDir /r deletes before the client ever runs. Payload dirs (plugins/translations/sounds/themes) stay in place to avoid restoring stale DLLs; restore + relocate handle the new backup set and the one-time notice covers both decks and folders. - Restart the %TEMP% backup when the 32/64-bit passes point at different dirs instead of merging (which restored one dir's files into the other); document that only the last pass relocates and the %TEMP% copy is not a durable second copy. Client: - Drop the portable at-risk warning: portable installs never shipped an uninstaller and portable updates preserve data/, so every portable user got a spurious dialog plus daily host-machine copies. - AtRiskDataWarning is now a plain class with Q_DECLARE_TR_FUNCTIONS instead of QObject+Q_OBJECT; logging category renamed to at_risk_data_warning; MainWindow wrapper inlined into startupConfigCheck. - Lower the synchronous startup backup cap 500 -> 200 MB. Utility/tests: - Remove test-only backupAtRiskPath()/findRecentBackupSession() and the no-marker/no-manifest legacy paths; isBackupSessionComplete() now requires the complete marker; tests updated to the plural API. * [Client] Restore collectInstalledAtRiskPaths per review a23e02b5 removed the definition along with the portable helpers, breaking the build (undefined reference at link time, and isInsideAnyDir unused under -Werror on Arch/Debian 13). Restores the 202c0d65 version (pathsInsideDir plus loose top-level deck files) as suggested. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-16 22:05:42 +02:00
BruebachL	456d9d9e70	[Server] Inform client of login/registration rate limit timeout (#7468) ... * [Security] Add per-address rate limiting for auth endpoints Introduce a thread-safe RateLimiter that tracks attempts per key (IP address) within a sliding time window, and wire it into the authentication endpoints: - Login: failed login attempts from an address are counted; once the configured maximum is exceeded within the window, further logins from that address are rejected with RespTooManyRequests. A successful login clears the failed attempts for that address. - Registration: implement the previously stubbed tooManyRegistrationAttempts, limiting how many accounts can be created per address per window. - Forgot-password: throttle both the email-request and the email-challenge paths per address. New [security] settings with defaults: max_login_attempts_per_ip=5 / login_attempt_window_seconds=900 max_registrations_per_ip=2 / registration_window_seconds=3600 max_forgot_password_requests_per_ip=3 / forgot_password_window_seconds=3600 Adds unit tests for the RateLimiter (window limit, over-limit blocking, clearing, per-key independence). Took 3 minutes * [Server] Fix rate limiter: disable semantics, draining window, bounded keys, pre-work login gate - recordAttempt()/isBlocked() return false when maxAttempts <= 0 or windowSeconds <= 0, so the documented 'set to 0 to disable' actually disables instead of blocking every address permanently - Keys at their limit are rejected without recording, so repeated attempts at the attacker's pace cannot keep sliding the window and holding the lockout open; a block now clears once the recorded attempts age out of windowSeconds - Attempts map is bounded: an opportunistic prune (every 60s) erases keys whose attempts have fully aged out - Login is throttled before the work: isLoginRateLimited() (a new virtual on Server, backed by isBlocked()) gates loginUser(), so a locked-out address no longer burns a database round trip and password verification per attempt - recordAttemptAt()/isBlockedAt() time seams make the sliding-window behaviour deterministically testable; tests rewritten for the real semantics (no reliance on the maxAttempts=0 bug, no sleeps) * [Server] Inform client of login/registration rate limit timeout - Add retryAfterSeconds/retryAfterSecondsAt to RateLimiter to compute seconds until oldest attempt exits the sliding window - Expose loginRateLimitRemainingSeconds via Server/Servatrice - Include denied_end_time (absolute epoch) in RespTooManyRequests for login pre-auth and when transitioning to lockout after failed attempt - Include denied_end_time in registration rate limit response - Client: show retry time for login RespTooManyRequests - Client: simplify registration RespTooManyRequests message to show retry time when provided - Add test for retryAfterSecondsAt * [Client] Restore registration guidance and drop duplicated rate limit reason The registration RespTooManyRequests case now only shows the retry-after copy when the server actually sent a denied_end_time; the per-email account cap path (no end time) falls back to the previous guidance about already having an account and contacting the server operator. Drop the canned denied_reason_str from the new login rate limit branches so the client no longer prints the same sentence twice. * [Server] Exempt trusted_sources from auth rate limits Addresses in security/trusted_sources bypass per-address limits, matching the existing max_users_per_address bypass in initSession. Without this, Webatrice behind local nginx (all clients as 127.0.0.1) would lock out every web user after 5 mistyped passwords, and cap the whole server at 2 registrations / 3 forgot-password requests per window. Adds Servatrice::isRateLimitExempt helper; recordFailedLogin, isLoginRateLimited, recordRegistrationAttempt and recordForgotPasswordAttempt all check it. # Conflicts: # servatrice/src/servatrice.cpp * [Server] Key login rate limit by (address, username) A successful login cleared the whole per-IP failure history, so 4 bad guesses plus 1 self-login (or guest UnknownUser login) from the same address bypassed the limiter. Failures are now bucketed per (address, lowercase username) and a success clears only its own pair. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-16 05:47:43 +02:00
BruebachL	fec693ca2a	[Tests] Scope loader matcher settings to the process to fix parallel-run flakes (#7504) ... * [Tests] Scope loader matcher settings to the process to fix parallel-run flakes * [Tests] Clean up per-process settings sandbox after loader matcher run * [Tests] Skip loader matcher sandbox cleanup during gtest discovery --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-15 12:21:40 +02:00
BruebachL	cce606764c	[Server] Resolve a zone dump request to an in-range index span (#7401) ... Command_DumpZone.number_cards is an unbounded sint32 supplied by the client, and cmdDumpZone used it as an index without ever putting it in range. for (int i = 0; (i < cards.size()) && (i < numberCards || numberCards == -1); ++i) { const auto &findId = cmd.is_reversed() ? cards.size() - numberCards + i : i; Server_Card *card = cards[findId]; A forward request happened to be safe, because the loop stopped at the zone's own size. A reversed one was not. number_cards is documented by the client as -1 for "the whole zone", and cards.size() - numberCards is therefore cardCount + 1, one past the last card. A request larger than the zone puts the offset at a negative index instead. Both read out of bounds, then dereference the result as a Server_Card, so either adjacent heap memory or a crash. Any player can reach it: the zone only has to be a public one, or the requester's own, and both number_cards and is_reversed come straight off the wire. A reversed dump of -1 is enough on any public zone with cards in it. Resolve the request to a half-open span up front. -1 and any positive count are clamped to what the zone actually holds, any other negative count resolves to nothing, and a reversed request takes the span from the end rather than computing an offset from a value that may not be in range. cards.at() then carries the bound as a second line of defence, and qsizetype keeps the loop and the list's own indexing type aligned. Also report card_count as the zone's real size. It was numberCards < cards.size() ? cards.size() : numberCards, which agrees with the zone size for every request that was in range and reported the client's own oversized number otherwise. The client uses card_count to draw placeholders when a zone lists no cards, so overstating it invents cards that were never there. Put the resolution in its own translation unit. server_abstract_player.cpp refers to the RNG global that only servatrice's main.cpp defines, so anything linking that object needs the whole server binary behind it, which is why no test could reach this code before. server_dump_zone_test covers each request shape and then asserts the span is contained in [0, cardCount) across the edges of the request domain, INT_MIN and INT_MAX included. Reverting the arithmetic makes three of them fail, the two out-of-bounds shapes and the containment sweep. The client's own copy of this convention at view_zone.cpp:93 already clamps the way this now does. Leaving that duplication for a follow-up, since the client reaching the same answer by a different route is not a defect. Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-13 11:23:13 +02:00
BruebachL	24874b10cf	[GameSelector] Persist quick-filter toolbar edits and repair filter settings migration (#7500) ... * [GameSelector] Persist quick-filter toolbar edits and repair filter settings migration - Save game-filter parameters on toolbar changes, debounced with flush on close - Repair pre-camelCase gamefilters.ini keys in place on startup - Map the legacy creator-name filter to hostNameFilter - Balance begin/endFilterChange in GamesProxyModel::refresh - Align the hideIgnoredUserGames default between model and settings - Cover the migration with regression tests * [Review] Address RickyRister comments on filter persistence - Rename game-type prefix constants to SCREAMING_SNAKE to match MIGRATION_SENTINEL_KEY and project style - Document why GameSelector destructor flushes a pending debounced filter save (toolbar path has no other writer) --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>	2026-10-13 07:45:08 +02:00