[9] - PDump Acore-CMS for Characters#206
Open
TheSCREWEDSoftware wants to merge 18 commits into
Open
Conversation
…nection logging + GeoIP
(Resume generated by Claude) **PR azerothcore#199 — Characters Menu** - Fixed typo: "succesfully" → "successfully" in save confirmation - Fixed broken sentence in the Order Character Screen description - Extended `user-select: none` to cover all `.acore-char-row` elements, not just `button` **PR azerothcore#200 — Mail Return** - *(issues covered by later PRs — no unique fixes)* **PR azerothcore#201 — Item Restoration** - Added `alt=` attributes to race/class icons in `CharactersView.php` - `ToolsApi`: `ItemRestoreList` now catches exceptions and returns a 403 instead of a 500 - `ToolsApi` + `ACoreServices`: **Security fix** — `ItemRestore` now verifies the recovery item belongs to the character, not just that the user owns the character name (IDOR vulnerability) **PR azerothcore#202 — Unstuck / Scroll / RAF** - `ItemRestorationPage`: removed brittle `data.includes('mail')` success check — any HTTP success now triggers the success path **PR azerothcore#203 — Security Tab** - `CharactersController`: `resetCharacterOrder()` now guards against invalid/null account ID before running UPDATE - `Tools.php`: added `esc_attr()` on banned-names table input (XSS fix) - `Tools.php`: added `is_array()` guard on thresholds `foreach` - `Tools.php` + `SettingsController`: added sentinel input so deleting all thresholds actually clears them in the DB - `Tools.php`: GeoIP hidden fallback `value="0"` is now always rendered, not conditionally **PR azerothcore#204 — Admin Settings / CSRF** - `AcoreCharColors`: removed unused `$fDark`, unknown race IDs now get a neutral color instead of Horde red - `MailReturnView`: added `esc_url()` on race/class icon `src` attributes - `DarkMode`: added `dmToggleInFlight` flag to prevent overlapping AJAX requests on rapid dark mode clicks
Forgot to add the toggle setting in azerothcore#205
📝 WalkthroughWalkthroughThe PR adds nonce checks and REST-backed admin tools, introduces character dump export flows, expands user security/profile features with 2FA and login history, and refreshes page layouts and styling across several admin and user-facing screens. ChangesAdmin settings and tools
Character export and ordering
User security and profile
User panel pages, dark mode, and styles
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
TheSCREWEDSoftware
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (22)
src/acore-wp-plugin/src/Hooks/Various/DarkMode.php-35-38 (1)
35-38: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBump the
acore-css(main.css) cache-buster version.
main.cssis substantially modified in this PR (new char-list, mail-return, and item-restoration layouts), yet it is still enqueued at version'3.0', whiledark-mode.cssandtheme.cssare bumped to'3.7'. Returning visitors with a cachedmain.csswill load stale styles against the new markup, producing broken layouts until a hard refresh.🔧 Proposed fix
- wp_enqueue_style('acore-css', ACORE_URL_PLG . 'web/assets/css/main.css', [], '3.0'); + wp_enqueue_style('acore-css', ACORE_URL_PLG . 'web/assets/css/main.css', [], '3.7');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Hooks/Various/DarkMode.php` around lines 35 - 38, The `DarkMode` hook is still enqueuing `acore-css` via `wp_enqueue_style` with an outdated cache-buster, so cached `main.css` can stay stale against the new markup. Update the version passed for `acore-css` to match the current release bump used by `acore-dark-mode` and `acore-theme`, keeping the change in the `DarkMode` enqueue block so returning visitors fetch the refreshed `main.css`.src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpApi.php-68-72 (1)
68-72: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not return internal warning details to users.
detailincludes exception messages plus file paths/line numbers. Log that server-side and return a correlation id or generic message to avoid leaking internals through the export UI.🛡️ Proposed response pattern
+ $errorId = wp_generate_uuid4(); + error_log('[acore-pdump][' . $errorId . '] ' . implode("\n", $phpWarnings)); wp_send_json_error([ 'message' => 'An internal error occurred while generating the dump.', - 'detail' => implode("\n", $phpWarnings), + 'detail' => 'Reference: ' . $errorId, ], 500);Also applies to: 156-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpApi.php` around lines 68 - 72, The error response in CharacterDumpApi::generateDump currently exposes internal PHP warning details through the detail field, including exception messages and file/line information. Update the wp_send_json_error paths in generateDump (and the other matching block noted in the comment) to stop returning raw warnings to the client; instead log the full warning data server-side and return only a generic message plus a correlation id or similar safe reference for support.src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php-374-379 (1)
374-379: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEscape dynamic values before writing modal HTML.
serverRevision,bugReportUrl, and character names flow intoinnerHTML; a crafted DB/config/name value can inject markup. Build DOM nodes or HTML-escape text before concatenation, and validate URLs before creating anchors.🛡️ Safer escaping pattern
+ function acoreEscapeHtml(value) { + return String(value) + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, '&`#039`;'); + } + function acorePdumpRevisionLink() { if (!acorePdumpRevision) return '(unknown revision)'; if (acorePdumpRevisionUrl) { - return '<a href="' + acorePdumpRevisionUrl + '" target="_blank" rel="noopener">' + acorePdumpRevision + '</a>'; + return '<a href="' + acoreEscapeHtml(acorePdumpRevisionUrl) + '" target="_blank" rel="noopener">' + acoreEscapeHtml(acorePdumpRevision) + '</a>'; } - return acorePdumpRevision; + return acoreEscapeHtml(acorePdumpRevision); }Also wrap
upper,c.name.toUpperCase(), andacorePdumpBugReportUrlwith the same escaping or create those nodes via DOM APIs.Also applies to: 387-407, 507-543
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php` around lines 374 - 379, The modal HTML built in CharactersView.php is using unescaped dynamic values, including acorePdumpRevisionLink, serverRevision, bugReportUrl, character names, and related upper/c.name values, which can be injected into innerHTML. Fix this by replacing string concatenation with DOM node creation or by HTML-escaping every text value before insertion, and validate acorePdumpBugReportUrl/acorePdumpRevisionUrl before using them in anchor hrefs. Apply the same escaping approach anywhere the modal content is assembled, especially in the character list and revision/bug-report sections referenced by the affected CharactersView logic.src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.php-459-462 (1)
459-462: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize SQL
NULLand empty strings distinctly.Returning
"'NULL'"writes the literal stringNULL, not SQLNULL; it also converts intentional empty strings intoNULLtext in the dump.🐛 Proposed fix
- if ($val === null || $val === '') { - return "'NULL'"; + if ($val === null) { + return "NULL"; + } + + if ($val === '') { + return "''"; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.php` around lines 459 - 462, The `CharacterDumpWriter::escapeValue` serializer is treating null/empty values incorrectly by emitting the quoted text `NULL` instead of a real SQL `NULL`, and it also collapses empty strings into the same output. Update `escapeValue` so it distinguishes `null` from `''`: return unquoted `NULL` for actual null values, preserve empty strings as empty quoted strings, and keep the existing escaping behavior for all other values.src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpApi.php-9-17 (1)
9-17: 🔒 Security & Privacy | 🟠 MajorGate PDUMP routes in
permission_callback
src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpApi.php:9-17still registers both PDUMP endpoints as public (__return_true). Move the login/feature check intopermission_callbackso unauthenticated or disabled-export requests are rejected before any handler/DB work runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpApi.php` around lines 9 - 17, The PDUMP REST endpoints are still exposed publicly because both register_rest_route calls in CharacterDumpApi currently use __return_true for permission_callback. Update the permission_callback logic for the handlePdump and handlePdumpAll routes to perform the login/feature gate check there, so unauthenticated or disabled-export requests are blocked before reaching the handlers or DB work.src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.php-420-437 (1)
420-437: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHandle escaped newline blobs when stripping custom channels.
The comment says the chat blob is stored with escaped
\n, but the regex only matches real newlines; custom channel names can remain in exported dumps.🛡️ Proposed fix
private static function stripCustomChannels(string $data): string { - $decoded = preg_replace_callback( + $usesEscapedNewlines = strpos($data, "\\n") !== false && strpos($data, "\n") === false; + $working = $usesEscapedNewlines ? str_replace("\\n", "\n", $data) : $data; + + $decoded = preg_replace_callback( '/^CHANNELS\n(.*?\n)END\n/ms', function (array $m): string { $kept = ''; @@ - $data + $working ); - return $decoded ?? $data; + if ($decoded === null) { + return $data; + } + + return $usesEscapedNewlines ? str_replace("\n", "\\n", $decoded) : $decoded; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.php` around lines 420 - 437, The CHANNELS stripping logic in CharacterDumpWriter::stripCustomChannels only matches real newline delimiters, so escaped “\n” blobs are missed and custom channels survive export. Update the regex/path in stripCustomChannels to handle the escaped-newline format used by the chat blob, then normalize or parse that blob before filtering against self::DEFAULT_CHANNELS so only default channels are kept. Keep the replacement behavior consistent with the current callback and ensure the returned data preserves the original blob format expected by the rest of the dump flow.src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpApi.php-126-154 (1)
126-154: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the error handler when bulk dump generation throws.
Unlike the single export path,
handlePdumpAll()does not catchgetDump()failures inside the loop, so an exception can skipob_end_clean()andrestore_error_handler().🛠️ Proposed cleanup structure
- ob_start(); - - $files = []; - foreach ($characters as $char) { + ob_start(); + $files = []; + try { + foreach ($characters as $char) { $guid = isset($char['guid']) ? (int) $char['guid'] : 0; if ($guid < 1) continue; ... - $writer = new CharacterDumpWriter($conn); - $dump = $writer->getDump($guid); + $writer = new CharacterDumpWriter($conn); + $dump = $writer->getDump($guid); if ($dump === null) continue; ... - $files["{$order}_{$level}_{$race}_{$class}_{$now}.dump"] = $dump; + $files["{$order}_{$level}_{$race}_{$class}_{$now}.dump"] = $dump; + } + } catch (\Throwable $e) { + $phpWarnings[] = get_class($e) . ': ' . $e->getMessage(); + } finally { + ob_end_clean(); + restore_error_handler(); } - - ob_end_clean(); - restore_error_handler();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpApi.php` around lines 126 - 154, The bulk export flow in handlePdumpAll() can leave output buffering and the temporary error handler active if CharacterDumpWriter::getDump() throws inside the loop. Wrap the ob_start()/foreach block in a try/finally so ob_end_clean() and restore_error_handler() always run, and keep the existing per-character logic and CharacterDumpWriter usage unchanged.src/acore-wp-plugin/web/assets/mail-return/mail-return.js-2-18 (1)
2-18: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard mail rendering against out-of-order responses.
The active card changes immediately, but the AJAX success handler renders whichever request finishes last. A slow response for the previous character can overwrite the current selection and produce return buttons with the wrong
data-char-guid.Proposed fix
+ var mailReturnRequestSeq = 0; + jQuery("`#acore-characters-mail`").on("click", ".acore-char-card", function () { var card = jQuery(this); var charGuid = card.data("char-guid"); + var requestSeq = ++mailReturnRequestSeq; @@ success: function (mails) { + if (requestSeq !== mailReturnRequestSeq) { + return; + } loading.hide(); @@ error: function (xhr, status, error) { + if (requestSeq !== mailReturnRequestSeq) { + return; + } loading.hide();Also applies to: 25-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/web/assets/mail-return/mail-return.js` around lines 2 - 18, Guard the mail-return rendering in mail-return.js against stale AJAX responses: the click handler on .acore-char-card updates the active selection immediately, but the request success path can still render an older character’s mails after a newer click. Update the request flow so each fetch in the acore-char-card handler tracks the current charGuid (or aborts/ignores prior requests) and only renders in the AJAX success callback when the response matches the latest selected card, ensuring the returned buttons keep the correct data-char-guid.src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php-114-130 (1)
114-130: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winIgnore stale item-list responses after character switches.
Line 120 starts a fetch tied to the clicked character, but the success handler renders unconditionally. If the user clicks character A then B quickly, A’s slower response can replace B’s grid and the restore buttons still carry A’s
characterName.Proposed fix
var listUrl = '<?= esc_js(get_rest_url(null, ACORE_SLUG . '/v1/item-restore/list/')) ?>'; var restoreUrl = '<?= esc_js(get_rest_url(null, ACORE_SLUG . '/v1/item-restore')) ?>'; var restNonce = '<?= esc_js(wp_create_nonce('wp_rest')) ?>'; + var itemListRequestSeq = 0; @@ function selectCharacter(guid, characterName) { + var requestSeq = ++itemListRequestSeq; resetState(); loading.style.display = 'block'; content.style.display = 'none'; grid.innerHTML = ''; fetch(listUrl + guid, { headers: { 'Accept': 'application/json', 'X-WP-Nonce': restNonce } }) .then(parseJsonResponse) .then(function (items) { + if (requestSeq !== itemListRequestSeq) { + return; + } loading.style.display = 'none'; @@ }) .catch(function (msg) { + if (requestSeq !== itemListRequestSeq) { + return; + } loading.style.display = 'none'; errorBox.textContent = msg && msg.message ? msg.message : String(msg); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php` around lines 114 - 130, The selectCharacter flow renders fetch results unconditionally, so a slower response from an earlier character selection can overwrite the current grid and restore actions. Update selectCharacter to track the latest requested character (for example with a request token or current selection state) and, inside the fetch/.then(parseJsonResponse) success path, verify the response still matches the active guid/characterName before updating loading, content, noResults, or rendering item actions. Make sure the restore buttons and any item state are built from the currently selected character only.src/acore-wp-plugin/src/Components/UserPanel/UserController.php-220-221 (1)
220-221: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAlign
getModel()with its declared contract.PHPStan reports this method is documented/expected to return
UserModel, but it now returns the controller instance. Either restore the model return value or update the public contract if controller-as-model is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/UserPanel/UserController.php` around lines 220 - 221, The UserController::getModel() implementation no longer matches its documented return contract: it currently returns the controller instance instead of a UserModel. Update getModel() to return the actual model object used by this controller, or if controller-as-model is intentional, adjust the method’s public PHPDoc/type declaration and any related contract so it consistently reflects the new return type.Source: Linters/SAST tools
src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php-10-11 (1)
10-11: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftApply these checks to the existing profile password-change path too.
This handler only protects the custom
acore_change_passwordform, while the existinguser_profile_update()path still processes$_POST['pass1']directly. Users can bypass the new 2FA unlock and password-history checks from the standard profile password form unless both paths share the same validation/change service or the default password form is disabled.Also applies to: 24-25, 60-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php` around lines 10 - 11, The password validation currently only runs in acore_process_security_password for the custom acore_change_password flow, so the standard profile update path can still change passwords through user_profile_update without the new checks. Refactor the shared password-change logic into a common service or validation method used by both acore_process_security_password and user_profile_update, and make sure the pass1-based profile form goes through the same 2FA unlock and password-history enforcement. If sharing is not possible, disable or block the default profile password form so only the validated flow can submit password changes.src/acore-wp-plugin/src/Components/UserPanel/UserController.php-96-125 (1)
96-125: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNormalize the account id before querying.
getAcoreAccountId()can return an empty/false value for users without a linked game account, but these queries interpolate it directly. Guard/cast once, then use bound parameters so RAF/item-restoration pages render an empty/error state instead of issuing malformed SQL.Suggested fix pattern
- $accId = $acServices->getAcoreAccountId(); - if (!isset($accId)) { + $accId = $acServices->getAcoreAccountId(); + $accId = is_numeric($accId) ? (int) $accId : 0; + if ($accId <= 0) { wp_die("<div class=\"notice notice-error\"><p>An error ocurred while loading your account information, please try again later. If this errors continues, please ask for support.</p></div>"); }- WHERE `account_id` = $accId + WHERE `account_id` = ? "; - $queryResult = $conn->executeQuery($query); + $queryResult = $conn->executeQuery($query, [$accId]);Also applies to: 138-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/UserPanel/UserController.php` around lines 96 - 125, Normalize the account id in UserController before any RAF queries by guarding the result of getAcoreAccountId() once and casting it to a safe integer/value only after validation. Then update the query execution in the UserController logic to use bound parameters instead of interpolating $accId directly in the SQL strings, so the recruit_a_friend_links and recruit_a_friend_rewards lookups fail gracefully with the existing empty/error state when no linked account is present.src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php-15-25 (1)
15-25: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd retention for stored login IP history.
This table stores IP addresses and login timestamps indefinitely. Add a configurable retention window and purge job, or avoid persisting more history than the UI/API actually expose.
Also applies to: 44-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php` around lines 15 - 25, The ConnectionLogger table currently stores login IP history indefinitely, so update the ConnectionLogger schema/logic to enforce a configurable retention window instead of unbounded storage. Add a purge mechanism (for example in the same User/ConnectionLogger flow or its related cron/hook registration) that deletes rows older than the configured retention period, and make sure the retention setting matches only what the UI/API actually need to expose. Also review the table definition and any insert/query paths tied to the login history table so the stored data lifecycle is consistent with the new retention policy.src/acore-wp-plugin/src/Hooks/User/User.php-759-764 (1)
759-764: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle self-removal log entries here too.
acore_2fa_sync_self_removals()stores website removals withby => selfand nostaff, so this notice can emit an undefined-index warning and incorrectly say staff removed the user’s 2FA. Mirror the fallback used inacore_profile_2fa_removal_warning().Proposed fix
- '<strong>' . esc_html($lastWebRemoval['staff']) . '</strong>' + (($lastWebRemoval['by'] ?? 'admin') === 'self' + ? '<strong>' . esc_html__('you', 'acore-wp-plugin') . '</strong>' + : '<strong>' . esc_html($lastWebRemoval['staff'] ?? __('a staff member', 'acore-wp-plugin')) . '</strong>')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Hooks/User/User.php` around lines 759 - 764, The website 2FA removal notice in User::show warnings for $lastWebRemoval assumes a staff value, but acore_2fa_sync_self_removals() can store self-removals with no staff field. Update the notice logic to mirror the fallback used in acore_profile_2fa_removal_warning(), so it safely handles self-removal entries by using a self-removal label instead of reading a missing staff index. Keep the existing formatting in the User class method, but branch on the removal source before building the message.src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php-228-230 (1)
228-230: 🔒 Security & Privacy | 🟠 MajorUse HTTPS for both GeoIP calls
src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php:228,302Both
ip-api.comrequests still send user IPs over plaintexthttp://. Switch them to an HTTPS-capable endpoint/provider, or gate GeoIP behind explicit admin opt-in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php` around lines 228 - 230, The GeoIP lookups in ConnectionLogger still send the user IP over plaintext HTTP, so update both ip-api.com requests in the relevant connection logging methods to use an HTTPS-capable endpoint or provider, or make GeoIP execution conditional on explicit admin opt-in. Use the existing ConnectionLogger logic around the two wp_remote_get calls as the place to update the URL scheme and related handling.src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php-123-135 (1)
123-135: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the resurrection module gate on save, not only in the UI.
The disabled select/hidden value prevents normal clicks, but the supplied save path persists posted options directly via
storeConf(). Forceacore_resurrection_scrollto0server-side whenmod-resurrection-scrollis absent so the stored config cannot drift from installed modules.🛡️ Suggested server-side guard
$modulesCsv = get_option('acore_modules_csv', ''); $installedModules = $modulesCsv ? array_map('trim', explode(',', $modulesCsv)) : []; $hasResurrectionMod = in_array('mod-resurrection-scroll', $installedModules, true); if ($key === 'acore_resurrection_scroll' && !$hasResurrectionMod) { $this->storeConf($key, '0'); continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php` around lines 123 - 135, The save path for resurrection scroll settings still accepts posted values even when the module is missing, so the config can drift from the UI state. Add a server-side guard in the save flow that handles acore_resurrection_scroll and checks the mod-resurrection-scroll gate before calling storeConf(). If the module is absent, force the stored value to 0 and skip any submitted enabled value; use the existing $hasResurrectionMod logic from Tools.php to keep the persisted config aligned with installed modules.src/acore-wp-plugin/src/Components/AdminPanel/Pages/PVPRewards.php-17-17 (1)
17-17: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid leaking the rewards nonce in preview URLs.
The Preview button switches this same form to
GET, so the nonce added on Line 17 is submitted into the query string. Disable nonce fields for preview and re-enable them for the POST reward action.🛡️ Proposed fix
jQuery('`#preview`').on('click', function (e) { - jQuery('`#pvp-rewards`').attr('method', 'GET'); + jQuery('`#pvp-rewards`') + .attr('method', 'GET') + .find('input[name="acore_pvp_rewards_nonce"], input[name="_wp_http_referer"]') + .prop('disabled', true); }); jQuery('`#send-rewards`').on('click', function (e) { - jQuery('`#pvp-rewards`').attr('method', 'POST'); + jQuery('`#pvp-rewards`') + .attr('method', 'POST') + .find('input[name="acore_pvp_rewards_nonce"], input[name="_wp_http_referer"]') + .prop('disabled', false); });Also applies to: 247-251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/PVPRewards.php` at line 17, The PVP rewards form is leaking the nonce into the query string when the preview action switches the form to GET. Update the `PVPRewards` page rendering so `wp_nonce_field('acore_pvp_rewards_save', 'acore_pvp_rewards_nonce')` is only emitted for the POST save/reward action, and skipped for preview submissions. Use the preview-related form/action logic in `PVPRewards` to gate the nonce field, and keep the nonce enabled only for the actual reward save flow.src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php-308-314 (1)
308-314: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse real buttons for threshold actions.
The Add/Reset/Delete controls are clickable
<div>elements, so keyboard users cannot tab to or activate them reliably.♿ Proposed fix
- <div id="acore-name-unlock-thresholds-add" class="button"> + <button type="button" id="acore-name-unlock-thresholds-add" class="button"> <span class="dashicons dashicons-plus" style="margin-top:5px;"></span> Add - </div> - <div id="acore-name-unlock-reset" class="button acore-btn-danger" title="Reset Name Unlock to defaults"> + </button> + <button type="button" id="acore-name-unlock-reset" class="button acore-btn-danger" title="Reset Name Unlock to defaults"> <span class="dashicons dashicons-image-rotate" style="margin-top:5px;"></span> Reset - </div> + </button>- const $btnDel = $(`<div class="button acore-btn-danger">`).appendTo($td); + const $btnDel = $('<button>', { type: 'button', class: 'button acore-btn-danger' }).appendTo($td);Also applies to: 603-605
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php` around lines 308 - 314, The Add/Reset/Delete threshold controls in the Tools page are implemented as clickable divs, which prevents reliable keyboard focus and activation. Update the threshold action elements in the relevant markup blocks (including the Add/Reset controls and the Delete control mentioned in the review) to use real button elements in the AdminPanel/Pages/Tools.php view, and preserve their existing ids, classes, and dashicon content so the existing behavior and styling hooks continue to work.src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php-641-650 (1)
641-650: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReset should restore default thresholds, not save an empty set.
Line 649 removes every threshold row, and the hidden sentinel then persists
acore_name_unlock_thresholdsas[]. That bypasses the defaults defined inOpts.🐛 Proposed fix
- 'This will clear the banned names table and delete all inactivity thresholds.\n\n' + + 'This will clear the banned names table and restore the default inactivity thresholds.\n\n' + 'This cannot be undone. Continue?', function () { $('input[name="acore_name_unlock_allowed_banned_names_table"]').val(''); $('`#acore-name-unlock-thresholds` tbody tr').remove(); + [[5, 30], [30, 90], [60, 180], [81, 360]].forEach(function (row, i) { + addThreshold(i, row[0], row[1]); + }); $('input[name="Submit"]').closest('form').submit(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php` around lines 641 - 650, The Reset Name Unlock action is clearing all threshold rows and then submitting an empty `acore_name_unlock_thresholds` value, which bypasses the defaults. Update the `#acore-name-unlock-reset` click handler in `Tools.php` so it restores the default threshold entries defined by `Opts` instead of removing every `#acore-name-unlock-thresholds tbody tr`; keep the form submission, but repopulate the thresholds UI with the default set before submit so the saved state matches the built-in defaults.src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php-451-456 (1)
451-456: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply the same attempt limit to direct TOTP removal.
verify-website-2farate-limits wrong TOTP codes, but this endpoint’s fresh-token path does not. A logged-in session without the code can brute-force the in-game 2FA removal gate.Proposed fix
$data = $request->get_json_params(); $token = isset($data['token']) ? trim((string) $data['token']) : ''; + $attemptKey = acore_2fa_attempt_key($user->ID); + if ((int) get_transient($attemptKey) >= 5) + return new \WP_Error('rate_limited', __('Too many incorrect codes. Please wait a few minutes.', 'acore-wp-plugin'), ['status' => 429]); if (!preg_match('/^\d{6}$/', $token)) return new \WP_Error('invalid_token', __('Please enter a valid 6-digit code.'), ['status' => 400]); - if (!\ACore\Components\ServerInfo\acore_wp2fa_code_is_valid($user->ID, $token)) + if (!\ACore\Components\ServerInfo\acore_wp2fa_code_is_valid($user->ID, $token)) { + set_transient($attemptKey, ((int) get_transient($attemptKey)) + 1, 10 * MINUTE_IN_SECONDS); return new \WP_Error('wrong_token', __('Incorrect code. Please try again.'), ['status' => 401]); + } + delete_transient($attemptKey);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php` around lines 451 - 456, The direct TOTP removal flow in ServerInfoApi::verify-website-2fa is missing the same attempt limiting used for wrong codes, so add the same per-user/session throttle before validating the fresh 6-digit token. Reuse the existing rate-limit/attempt-tracking logic from the 2FA verification path around the token checks in this method, and return the same failure response once the limit is exceeded so direct removal cannot be brute-forced.src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php-152-158 (1)
152-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
$userIdwhen checking in-game 2FA.This helper ignores its parameter and always checks
getAcoreAccountId()for the current session. Callers that pass a target profile user can show the wrong in-game 2FA state.Proposed fix
function acore_ingame_2fa_enabled(int $userId): bool { try { - $services = \ACore\Manager\ACoreServices::I(); - $accId = $services->getAcoreAccountId(); - if (!$accId) return false; - $conn = $services->getAccountEm()->getConnection(); - $row = $conn->executeQuery('SELECT totp_secret FROM account WHERE id = ?', [$accId])->fetchAssociative(); + $wpUser = get_user_by('id', $userId); + if (!$wpUser) return false; + $conn = \ACore\Manager\ACoreServices::I()->getAccountEm()->getConnection(); + $row = $conn->executeQuery('SELECT totp_secret FROM account WHERE username = ?', [strtoupper($wpUser->user_login)])->fetchAssociative(); return $row && $row['totp_secret'] !== null; } catch (\Throwable $e) { return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php` around lines 152 - 158, The acore_ingame_2fa_enabled helper is ignoring its $userId argument and always using the current session account from ACoreServices::I()->getAcoreAccountId(), which can return the wrong 2FA state for profile-specific lookups. Update acore_ingame_2fa_enabled to resolve the account identity from the passed $userId instead of the session account, and use that resolved account when querying the account table so callers like ServerInfoApi get the correct in-game 2FA status.Source: Linters/SAST tools
src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php-272-280 (1)
272-280: 🎯 Functional Correctness | 🟠 MajorSend the REST nonce with the SOAP status check. This
$.get()relies on cookie auth, so withoutX-WP-NonceWordPress can treat the request as unauthenticated and fail themanage_optionscheck, making valid admins see SOAP as offline. Use the same nonce header as the POST requests below.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php` around lines 272 - 280, The SOAP status check in checkSoap() is missing the REST nonce header, so the request can be treated as unauthenticated. Update the $.get call used for server-info to send the same X-WP-Nonce header as the POST requests below, keeping the existing manual button state handling and setSoapStatus() flow unchanged.
🧹 Nitpick comments (2)
src/acore-wp-plugin/src/Hooks/Various/DarkMode.php (1)
51-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider consolidating the late inline overrides with the stylesheets.
The card,
.mail-entry,.form-select,hr, and myCred rules injected here largely duplicate rules already present indark-mode.css/theme.css. Maintaining two copies (one inline, one in a file) invites silent divergence. Sincetheme.cssis already enqueued last specifically "so it wins", most of these!importantoverrides could live there instead, keepingadmin_headreserved for genuinely cascade-sensitive cases (e.g. the WP2FA modal sizing).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/src/Hooks/Various/DarkMode.php` around lines 51 - 105, The late inline CSS in acore_dark_mode_late_styles duplicates rules already maintained in dark-mode.css/theme.css, so move the shared overrides for card, .mail-entry, .form-select, hr, and myCred into the stylesheet(s) and keep only truly cascade-sensitive inline styles here. Use acore_dark_mode_late_styles and the existing dark-mode.css/theme.css enqueue order to locate the affected rules, and preserve the WP2FA modal sizing inline if it still needs to be injected via admin_head.src/acore-wp-plugin/web/assets/css/dark-mode.css (1)
526-533: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSelector matching inconsistency for myCred.
This rule uses
[class^="mycred"](starts-with), whereastheme.cssandDarkMode.phptarget[class*="mycred"](contains). Elements wheremycred*is not the first class token won't be matched here, so the dark styling can be applied inconsistently across the two files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acore-wp-plugin/web/assets/css/dark-mode.css` around lines 526 - 533, The myCred dark-mode selector set is inconsistent with the rest of the theme because `body.acore-dark-mode [class^="mycred"]` only matches when the class starts with that prefix. Update the selector in the dark-mode stylesheet to use the same contains-based matching as `theme.css` and `DarkMode.php`, keeping the `body.acore-dark-mode` scope and the existing `.mycred-wrap` and `.mycred-widget` rules aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.php`:
- Around line 33-40: The CharacterDumpWriter class uses PHP 7.4-only typed
property declarations, which conflicts with the stated PHP 7.1 support. Update
the class properties in CharacterDumpWriter to avoid typed properties and any
other PHP 7.4-only syntax, or else explicitly raise the plugin’s PHP requirement
to 7.4+ wherever the runtime/support version is declared. Check the constructor
and any methods relying on $items, $mails, $pets, $race, $fakeGuid, and
$fakeAccount to ensure they still initialize and use the untyped properties
correctly.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php`:
- Around line 241-243: The SecurityPage WP2FA section is still rendering
protected controls from $wp2faHtml before the unlock flow completes, even though
the panel is hidden with CSS. Update the SecurityPage rendering branch so the
unlocked-only path controlled by $twofaUnlocked is the only place that outputs
WP2FA controls, including backup-code/settings markup and nonces. Use a
placeholder in the locked state and, after successful TOTP/email verification,
reload the page instead of revealing pre-rendered content.
In `@src/acore-wp-plugin/src/Hooks/User/User.php`:
- Around line 834-835: Replace the arrow function assigned to $fmtDate in the
User::... logic with a traditional anonymous function so the code remains
compatible with PHP 7.1. Locate the closure near the $chars_url assignment in
the User hook class and convert fn($ts) => ... into function ($ts) { ... };
without changing the formatting behavior.
---
Major comments:
In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/PVPRewards.php`:
- Line 17: The PVP rewards form is leaking the nonce into the query string when
the preview action switches the form to GET. Update the `PVPRewards` page
rendering so `wp_nonce_field('acore_pvp_rewards_save',
'acore_pvp_rewards_nonce')` is only emitted for the POST save/reward action, and
skipped for preview submissions. Use the preview-related form/action logic in
`PVPRewards` to gate the nonce field, and keep the nonce enabled only for the
actual reward save flow.
In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php`:
- Around line 272-280: The SOAP status check in checkSoap() is missing the REST
nonce header, so the request can be treated as unauthenticated. Update the $.get
call used for server-info to send the same X-WP-Nonce header as the POST
requests below, keeping the existing manual button state handling and
setSoapStatus() flow unchanged.
In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php`:
- Around line 123-135: The save path for resurrection scroll settings still
accepts posted values even when the module is missing, so the config can drift
from the UI state. Add a server-side guard in the save flow that handles
acore_resurrection_scroll and checks the mod-resurrection-scroll gate before
calling storeConf(). If the module is absent, force the stored value to 0 and
skip any submitted enabled value; use the existing $hasResurrectionMod logic
from Tools.php to keep the persisted config aligned with installed modules.
- Around line 308-314: The Add/Reset/Delete threshold controls in the Tools page
are implemented as clickable divs, which prevents reliable keyboard focus and
activation. Update the threshold action elements in the relevant markup blocks
(including the Add/Reset controls and the Delete control mentioned in the
review) to use real button elements in the AdminPanel/Pages/Tools.php view, and
preserve their existing ids, classes, and dashicon content so the existing
behavior and styling hooks continue to work.
- Around line 641-650: The Reset Name Unlock action is clearing all threshold
rows and then submitting an empty `acore_name_unlock_thresholds` value, which
bypasses the defaults. Update the `#acore-name-unlock-reset` click handler in
`Tools.php` so it restores the default threshold entries defined by `Opts`
instead of removing every `#acore-name-unlock-thresholds tbody tr`; keep the
form submission, but repopulate the thresholds UI with the default set before
submit so the saved state matches the built-in defaults.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpApi.php`:
- Around line 68-72: The error response in CharacterDumpApi::generateDump
currently exposes internal PHP warning details through the detail field,
including exception messages and file/line information. Update the
wp_send_json_error paths in generateDump (and the other matching block noted in
the comment) to stop returning raw warnings to the client; instead log the full
warning data server-side and return only a generic message plus a correlation id
or similar safe reference for support.
- Around line 9-17: The PDUMP REST endpoints are still exposed publicly because
both register_rest_route calls in CharacterDumpApi currently use __return_true
for permission_callback. Update the permission_callback logic for the
handlePdump and handlePdumpAll routes to perform the login/feature gate check
there, so unauthenticated or disabled-export requests are blocked before
reaching the handlers or DB work.
- Around line 126-154: The bulk export flow in handlePdumpAll() can leave output
buffering and the temporary error handler active if
CharacterDumpWriter::getDump() throws inside the loop. Wrap the
ob_start()/foreach block in a try/finally so ob_end_clean() and
restore_error_handler() always run, and keep the existing per-character logic
and CharacterDumpWriter usage unchanged.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.php`:
- Around line 459-462: The `CharacterDumpWriter::escapeValue` serializer is
treating null/empty values incorrectly by emitting the quoted text `NULL`
instead of a real SQL `NULL`, and it also collapses empty strings into the same
output. Update `escapeValue` so it distinguishes `null` from `''`: return
unquoted `NULL` for actual null values, preserve empty strings as empty quoted
strings, and keep the existing escaping behavior for all other values.
- Around line 420-437: The CHANNELS stripping logic in
CharacterDumpWriter::stripCustomChannels only matches real newline delimiters,
so escaped “\n” blobs are missed and custom channels survive export. Update the
regex/path in stripCustomChannels to handle the escaped-newline format used by
the chat blob, then normalize or parse that blob before filtering against
self::DEFAULT_CHANNELS so only default channels are kept. Keep the replacement
behavior consistent with the current callback and ensure the returned data
preserves the original blob format expected by the rest of the dump flow.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php`:
- Around line 374-379: The modal HTML built in CharactersView.php is using
unescaped dynamic values, including acorePdumpRevisionLink, serverRevision,
bugReportUrl, character names, and related upper/c.name values, which can be
injected into innerHTML. Fix this by replacing string concatenation with DOM
node creation or by HTML-escaping every text value before insertion, and
validate acorePdumpBugReportUrl/acorePdumpRevisionUrl before using them in
anchor hrefs. Apply the same escaping approach anywhere the modal content is
assembled, especially in the character list and revision/bug-report sections
referenced by the affected CharactersView logic.
In `@src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php`:
- Around line 451-456: The direct TOTP removal flow in
ServerInfoApi::verify-website-2fa is missing the same attempt limiting used for
wrong codes, so add the same per-user/session throttle before validating the
fresh 6-digit token. Reuse the existing rate-limit/attempt-tracking logic from
the 2FA verification path around the token checks in this method, and return the
same failure response once the limit is exceeded so direct removal cannot be
brute-forced.
- Around line 152-158: The acore_ingame_2fa_enabled helper is ignoring its
$userId argument and always using the current session account from
ACoreServices::I()->getAcoreAccountId(), which can return the wrong 2FA state
for profile-specific lookups. Update acore_ingame_2fa_enabled to resolve the
account identity from the passed $userId instead of the session account, and use
that resolved account when querying the account table so callers like
ServerInfoApi get the correct in-game 2FA status.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php`:
- Around line 114-130: The selectCharacter flow renders fetch results
unconditionally, so a slower response from an earlier character selection can
overwrite the current grid and restore actions. Update selectCharacter to track
the latest requested character (for example with a request token or current
selection state) and, inside the fetch/.then(parseJsonResponse) success path,
verify the response still matches the active guid/characterName before updating
loading, content, noResults, or rendering item actions. Make sure the restore
buttons and any item state are built from the currently selected character only.
In `@src/acore-wp-plugin/src/Components/UserPanel/UserController.php`:
- Around line 220-221: The UserController::getModel() implementation no longer
matches its documented return contract: it currently returns the controller
instance instead of a UserModel. Update getModel() to return the actual model
object used by this controller, or if controller-as-model is intentional, adjust
the method’s public PHPDoc/type declaration and any related contract so it
consistently reflects the new return type.
- Around line 96-125: Normalize the account id in UserController before any RAF
queries by guarding the result of getAcoreAccountId() once and casting it to a
safe integer/value only after validation. Then update the query execution in the
UserController logic to use bound parameters instead of interpolating $accId
directly in the SQL strings, so the recruit_a_friend_links and
recruit_a_friend_rewards lookups fail gracefully with the existing empty/error
state when no linked account is present.
In `@src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php`:
- Around line 15-25: The ConnectionLogger table currently stores login IP
history indefinitely, so update the ConnectionLogger schema/logic to enforce a
configurable retention window instead of unbounded storage. Add a purge
mechanism (for example in the same User/ConnectionLogger flow or its related
cron/hook registration) that deletes rows older than the configured retention
period, and make sure the retention setting matches only what the UI/API
actually need to expose. Also review the table definition and any insert/query
paths tied to the login history table so the stored data lifecycle is consistent
with the new retention policy.
- Around line 228-230: The GeoIP lookups in ConnectionLogger still send the user
IP over plaintext HTTP, so update both ip-api.com requests in the relevant
connection logging methods to use an HTTPS-capable endpoint or provider, or make
GeoIP execution conditional on explicit admin opt-in. Use the existing
ConnectionLogger logic around the two wp_remote_get calls as the place to update
the URL scheme and related handling.
In `@src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php`:
- Around line 10-11: The password validation currently only runs in
acore_process_security_password for the custom acore_change_password flow, so
the standard profile update path can still change passwords through
user_profile_update without the new checks. Refactor the shared password-change
logic into a common service or validation method used by both
acore_process_security_password and user_profile_update, and make sure the
pass1-based profile form goes through the same 2FA unlock and password-history
enforcement. If sharing is not possible, disable or block the default profile
password form so only the validated flow can submit password changes.
In `@src/acore-wp-plugin/src/Hooks/User/User.php`:
- Around line 759-764: The website 2FA removal notice in User::show warnings for
$lastWebRemoval assumes a staff value, but acore_2fa_sync_self_removals() can
store self-removals with no staff field. Update the notice logic to mirror the
fallback used in acore_profile_2fa_removal_warning(), so it safely handles
self-removal entries by using a self-removal label instead of reading a missing
staff index. Keep the existing formatting in the User class method, but branch
on the removal source before building the message.
In `@src/acore-wp-plugin/src/Hooks/Various/DarkMode.php`:
- Around line 35-38: The `DarkMode` hook is still enqueuing `acore-css` via
`wp_enqueue_style` with an outdated cache-buster, so cached `main.css` can stay
stale against the new markup. Update the version passed for `acore-css` to match
the current release bump used by `acore-dark-mode` and `acore-theme`, keeping
the change in the `DarkMode` enqueue block so returning visitors fetch the
refreshed `main.css`.
In `@src/acore-wp-plugin/web/assets/mail-return/mail-return.js`:
- Around line 2-18: Guard the mail-return rendering in mail-return.js against
stale AJAX responses: the click handler on .acore-char-card updates the active
selection immediately, but the request success path can still render an older
character’s mails after a newer click. Update the request flow so each fetch in
the acore-char-card handler tracks the current charGuid (or aborts/ignores prior
requests) and only renders in the AJAX success callback when the response
matches the latest selected card, ensuring the returned buttons keep the correct
data-char-guid.
---
Nitpick comments:
In `@src/acore-wp-plugin/src/Hooks/Various/DarkMode.php`:
- Around line 51-105: The late inline CSS in acore_dark_mode_late_styles
duplicates rules already maintained in dark-mode.css/theme.css, so move the
shared overrides for card, .mail-entry, .form-select, hr, and myCred into the
stylesheet(s) and keep only truly cascade-sensitive inline styles here. Use
acore_dark_mode_late_styles and the existing dark-mode.css/theme.css enqueue
order to locate the affected rules, and preserve the WP2FA modal sizing inline
if it still needs to be injected via admin_head.
In `@src/acore-wp-plugin/web/assets/css/dark-mode.css`:
- Around line 526-533: The myCred dark-mode selector set is inconsistent with
the rest of the theme because `body.acore-dark-mode [class^="mycred"]` only
matches when the class starts with that prefix. Update the selector in the
dark-mode stylesheet to use the same contains-based matching as `theme.css` and
`DarkMode.php`, keeping the `body.acore-dark-mode` scope and the existing
`.mycred-wrap` and `.mycred-widget` rules aligned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9bf371c-9810-4b46-93c6-24f01ef1faf8
📒 Files selected for processing (38)
src/acore-wp-plugin/src/Components/AdminPanel/Pages/ElunaSettings.phpsrc/acore-wp-plugin/src/Components/AdminPanel/Pages/PVPRewards.phpsrc/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.phpsrc/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.phpsrc/acore-wp-plugin/src/Components/AdminPanel/SettingsController.phpsrc/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpApi.phpsrc/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.phpsrc/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.phpsrc/acore-wp-plugin/src/Components/CharactersMenu/CharactersMenu.phpsrc/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.phpsrc/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnController.phpsrc/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.phpsrc/acore-wp-plugin/src/Components/ResurrectionScrollMenu/ResurrectionScrollMenu.phpsrc/acore-wp-plugin/src/Components/ResurrectionScrollMenu/ResurrectionScrollView.phpsrc/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.phpsrc/acore-wp-plugin/src/Components/Tools/ToolsApi.phpsrc/acore-wp-plugin/src/Components/UnstuckMenu/UnstuckView.phpsrc/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.phpsrc/acore-wp-plugin/src/Components/UserPanel/Pages/RafProgressPage.phpsrc/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.phpsrc/acore-wp-plugin/src/Components/UserPanel/UserController.phpsrc/acore-wp-plugin/src/Components/UserPanel/UserMenu.phpsrc/acore-wp-plugin/src/Components/UserPanel/UserView.phpsrc/acore-wp-plugin/src/Hooks/User/ConnectionLogger.phpsrc/acore-wp-plugin/src/Hooks/User/Include.phpsrc/acore-wp-plugin/src/Hooks/User/PasswordHandler.phpsrc/acore-wp-plugin/src/Hooks/User/User.phpsrc/acore-wp-plugin/src/Hooks/Various/DarkMode.phpsrc/acore-wp-plugin/src/Hooks/Various/tgmplugin_activator.phpsrc/acore-wp-plugin/src/Manager/ACoreServices.phpsrc/acore-wp-plugin/src/Manager/Opts.phpsrc/acore-wp-plugin/src/Manager/Soap/SmartstoneService.phpsrc/acore-wp-plugin/src/Utils/AcoreCharColors.phpsrc/acore-wp-plugin/src/boot.phpsrc/acore-wp-plugin/web/assets/css/dark-mode.csssrc/acore-wp-plugin/web/assets/css/main.csssrc/acore-wp-plugin/web/assets/css/theme.csssrc/acore-wp-plugin/web/assets/mail-return/mail-return.js
Comment on lines
+33
to
+40
| private array $items = []; | ||
| private array $mails = []; | ||
| private array $pets = []; | ||
|
|
||
| private int $race = 0; | ||
|
|
||
| private int $fakeGuid = 0; | ||
| private int $fakeAccount = 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the declared PHP constraint and locate PHP 7.4-only syntax in this file.
rg -n '"php"|requires|platform' composer.json . --glob 'composer.json' --glob '!vendor/**'
rg -n 'private\s+(array|int|string|bool|\?)|[0-9]_[0-9]' src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.phpRepository: azerothcore/acore-cms
Length of output: 3958
Remove PHP 7.4-only syntax or raise the PHP requirement
src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.php declares PHP 7.1 support, but the typed properties at lines 33-40 and the numeric separators at lines 172-173 require PHP 7.4+, so the plugin won’t parse on the stated runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharacterDumpWriter.php`
around lines 33 - 40, The CharacterDumpWriter class uses PHP 7.4-only typed
property declarations, which conflicts with the stated PHP 7.1 support. Update
the class properties in CharacterDumpWriter to avoid typed properties and any
other PHP 7.4-only syntax, or else explicitly raise the plugin’s PHP requirement
to 7.4+ wherever the runtime/support version is declared. Check the constructor
and any methods relying on $items, $mails, $pets, $race, $fakeGuid, and
$fakeAccount to ensure they still initialize and use the untyped properties
correctly.
Comment on lines
+241
to
+243
| <div id="acore-2fa-panel" style="display:none; margin-top:16px;"> | ||
| <?= $wp2faHtml ?> | ||
| </div> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not render protected WP2FA controls before unlock.
The TOTP/email gate only hides $wp2faHtml with display:none; the backup-code/settings markup and any form nonces are still delivered to the browser before verification. Render a placeholder here and reload after successful unlock so the server-side $twofaUnlocked branch is the only path that outputs WP2FA controls.
Proposed direction
- <div id="acore-2fa-panel" style="display:none; margin-top:16px;">
- <?= $wp2faHtml ?>
- </div>
+ <div id="acore-2fa-panel" style="display:none; margin-top:16px;"></div>After successful TOTP/email verification, reload instead of only revealing the pre-rendered panel:
- revealPanel();
+ window.location.reload();Also applies to: 265-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php` around
lines 241 - 243, The SecurityPage WP2FA section is still rendering protected
controls from $wp2faHtml before the unlock flow completes, even though the panel
is hidden with CSS. Update the SecurityPage rendering branch so the
unlocked-only path controlled by $twofaUnlocked is the only place that outputs
WP2FA controls, including backup-code/settings markup and nonces. Use a
placeholder in the locked state and, after successful TOTP/email verification,
reload the page instead of revealing pre-rendered content.
Comment on lines
+834
to
+835
| $chars_url = admin_url('profile.php?page=' . ACORE_SLUG . '-characters-menu'); | ||
| $fmtDate = fn($ts) => date('jS \o\f F, Y \a\t H:i', intval($ts)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declared PHP constraints and find arrow functions that would break PHP 7.1.
fd 'composer\.(json|lock)$' . --exec sed -n '1,120p' {}
rg -nP '\bfn\s*\(' src/acore-wp-plugin/src/Hooks/User/User.phpRepository: azerothcore/acore-cms
Length of output: 47961
Replace the arrow function with a normal closure. fn($ts) => ... requires PHP 7.4+, but this plugin supports PHP 7.1, so it will fatal on supported installs. Use function ($ts) { ... }; instead.
src/acore-wp-plugin/src/Hooks/User/User.php:835
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/acore-wp-plugin/src/Hooks/User/User.php` around lines 834 - 835, Replace
the arrow function assigned to $fmtDate in the User::... logic with a
traditional anonymous function so the code remains compatible with PHP 7.1.
Locate the closure near the $chars_url assignment in the User hook class and
convert fn($ts) => ... into function ($ts) { ... }; without changing the
formatting behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is not a Split from the main PR: #197
But the Git history is based on the last commit of #204, this commit: TheSCREWEDSoftware@f581aba it's the only one that adds what missing on that PR (which is a toggle setting turn on and off and chosoe which specific setting you want, account mute, account bans or character bans).
This PR mainly focus in allowing the user to have the equivalent
.pdump writein character screen in acore-cms.This uses the same code logic as azerothcore just keeps persona identifiable information anomylaised.
PDUMP automatically (when given an account) gives the account ID and GUID
Horde goes to Orgrimmar
Alliane goes to Stormwind
Invalid race or faction it goes to Booty Bay, this gurantees that there's always a place to go and not the original place of the character
This behaves as Pdump command does, the only difference changes the position and removes any PII from the dump file, ensuring there's no information back to the player.