[8] - Show ban and mute info to users#205
Open
TheSCREWEDSoftware wants to merge 15 commits into
Open
Conversation
…nection logging + GeoIP
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds security and 2FA flows, login-history tracking, module-aware admin tooling, a dark-mode toggle and shared styling updates, plus refreshed card-based UI for item restoration, mail return, unstuck, RAF, and character ordering. ChangesSecurity, admin tooling, and UI refresh
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
![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 (20)
src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php-272-280 (1)
272-280: 🎯 Functional Correctness | 🟠 MajorSend the REST nonce with this status check.
server-infois gated bymanage_options, but this$.get()is the only REST call here that skipsX-WP-Nonce. WordPress cookie auth can treat it as unauthenticated and hide the SOAP/modules panels for logged-in admins.🤖 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 checkSoap status request is missing the REST nonce, so the `server-info` call can be treated as unauthenticated for logged-in admins. Update the `checkSoap` function in `RealmSettings.php` so the `$.get(restBase + 'server-info', ...)` request includes `X-WP-Nonce` the same way the other REST calls do, using the existing nonce value already available in this admin page. Keep the existing success/fail/always behavior in `setSoapStatus` and the `manual` button state handling unchanged.src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php-220-243 (1)
220-243: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not render the protected WP2FA panel before server-side unlock.
The gate is client-side only because
$wp2faHtmlis already present in the hidden DOM. Anyone with the page can reveal or inspect the backup-code/settings UI without completing the fresh 2FA check. Render the panel only when$twofaUnlockedis true; after successful verification, reload so the server-side transient controls rendering.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>- if (data && data.success) { - revealPanel(); + if (data && data.success) { + window.location.reload();Apply the same pattern to the email gate branch.
Also applies to: 244-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 220 - 243, The WP2FA management panel is still being rendered into the DOM in the gated branch, so the unlock is only cosmetic and can be bypassed by inspecting hidden HTML. Update the SecurityPage rendering logic so $wp2faHtml is emitted only when $twofaUnlocked is true, and keep the gated branch from outputting the protected panel until a server-side unlock has occurred. After a successful TOTP verification in the related unlock flow, reload the page so the transient-controlled $twofaUnlocked state can render the panel, and apply the same server-side gating pattern to the email-gate branch.src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php-200-208 (1)
200-208: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle non-string SOAP failures before
stripos.
ServerInfoApi::serverInfo()can return anException/non-string response, but Line 207 passes it tostripos(), which can fatal instead of returning the intended 503.Proposed fix
$result = ServerInfoApi::serverInfo(); + if ($result instanceof \Throwable) { + error_log('[acore] server-info SOAP error: ' . $result->getMessage()); + return new \WP_Error('soap_error', __('Server information is temporarily unavailable.', 'acore-wp-plugin'), ['status' => 503]); + } + if (!is_string($result)) { + error_log('[acore] server-info SOAP error: non-string response'); + return new \WP_Error('soap_error', __('Server information is temporarily unavailable.', 'acore-wp-plugin'), ['status' => 503]); + } $errorPatterns = [🤖 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 200 - 208, ServerInfoApi::serverInfo() may return an Exception or other non-string value, so the SOAP error detection loop must guard before calling stripos(). In the server info handling block, normalize or branch on the result type first, then only run the errorPatterns matching for string responses; for non-string failures, log them and return the intended 503 path directly. Use the existing ServerInfoApi::serverInfo, $result, and $errorPatterns block to locate the fix.src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php-447-456 (1)
447-456: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winApply the same attempt limiter to direct in-game 2FA removal verification.
This fresh-token path bypasses the 5-attempt transient used by
verify-website-2fa, so repeated wrong TOTP submissions to this endpoint are not rate-limited.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 447 - 456, The fresh-token branch in ServerInfoApi’s in-game 2FA removal flow is missing the same 5-attempt limiter used by verify-website-2fa, so repeated wrong TOTP submissions are not rate-limited. Update the direct verification path around acore_wp2fa_code_is_valid to reuse the existing attempt-tracking transient logic (increment on failure, block after 5, reset on success) with the same user-scoped key pattern used elsewhere in ServerInfoApi. Keep the authorization flow unchanged, but ensure both the unlock-based and fresh-token paths enforce the same retry protection before returning wrong_token.src/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.php-56-61 (1)
56-61: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude
unbandate = bandatepermanent account bans.The character-ban query treats
unbandate = bandateas permanent, but this account-ban query filters those rows out, so permanent account bans can be missed.Proposed fix
- AND (`unbandate` = 0 OR `unbandate` > UNIX_TIMESTAMP()) + AND (`unbandate` = 0 OR `unbandate` = `bandate` OR `unbandate` > UNIX_TIMESTAMP())🤖 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/CharactersController.php` around lines 56 - 61, The account-ban lookup in CharactersController::... currently excludes permanent bans because the query only accepts unbandate = 0 or future timestamps. Update the account_banned query used for $accBanRow so it also treats unbandate = bandate as a permanent ban, matching the character-ban logic. Keep the existing active and latest-row behavior intact while broadening the WHERE clause to include this permanent-ban case.src/acore-wp-plugin/src/Components/CharactersMenu/CharactersMenu.php-35-39 (1)
35-39: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMirror the permanent-ban predicate in the menu badge query.
Without
unbandate = bandate, permanent account bans encoded that way will not show the redBannedbadge in the submenu.Proposed fix
- AND (`unbandate` = 0 OR `unbandate` > UNIX_TIMESTAMP()) + AND (`unbandate` = 0 OR `unbandate` = `bandate` OR `unbandate` > UNIX_TIMESTAMP())🤖 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/CharactersMenu.php` around lines 35 - 39, The CharactersMenu badge query is missing the permanent-ban condition used elsewhere, so accounts banned with matching unbandate and bandate won’t be marked banned. Update the query in CharactersMenu::executeQuery usage to mirror the existing permanent-ban predicate by adding the unbandate = bandate check alongside the current active and unbandate timestamp logic, keeping the badge state consistent for all permanent bans.src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php-40-42 (1)
40-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat
unbandate = bandateas a permanent account ban here too.If the account-ban query includes permanent rows with equal
bandate/unbandate, this view currently renders them as a temporary ban with0 secondsremaining instead ofBanned.Proposed fix
// Account ban $isAccountBanned = !empty($accBanRow); - $accBanPerma = $isAccountBanned && intval($accBanRow['unbandate']) === 0; - $accBanRemaining = ($isAccountBanned && !$accBanPerma) ? max(0, intval($accBanRow['unbandate']) - $now) : 0; + $accBanBandate = $isAccountBanned ? intval($accBanRow['bandate']) : 0; + $accBanUnbandate = $isAccountBanned ? intval($accBanRow['unbandate']) : 0; + $accBanPerma = $isAccountBanned && ($accBanUnbandate === 0 || $accBanUnbandate === $accBanBandate); + $accBanRemaining = ($isAccountBanned && !$accBanPerma) ? max(0, $accBanUnbandate - $now) : 0;🤖 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 40 - 42, The account-ban rendering logic in CharactersView treats only unbandate = 0 as permanent, so equal bandate/unbandate rows still show as a temporary ban with 0 seconds remaining. Update the ban-state checks around $isAccountBanned, $accBanPerma, and $accBanRemaining so that rows where unbandate equals bandate are also classified as permanent, and ensure the UI path that displays “Banned” is used for that case instead of the remaining-time display.src/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.php-31-46 (1)
31-46: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard missing account IDs before building the character query.
getAcoreAccountId()can return no row; interpolating that intoc.account = $accIdcan produce malformed SQL for users without a linked game account. Validate once and bind it like the other queries.Proposed fix
- $accId = ACoreServices::I()->getAcoreAccountId(); + $accId = ACoreServices::I()->getAcoreAccountId(); + if (!isset($accId) || $accId === '' || !is_numeric($accId)) { + wp_die('<div class="notice notice-error"><p>Unable to load your game account information.</p></div>'); + } + $accId = intval($accId); $conn = ACoreServices::I()->getCharacterEm()->getConnection(); @@ - WHERE c.`deleteDate` IS NULL AND c.`account` = $accId + WHERE c.`deleteDate` IS NULL AND c.`account` = ? ORDER BY COALESCE(c.`order`, c.`guid`) "; - $chars = $conn->executeQuery($query)->fetchAllAssociative(); + $chars = $conn->executeQuery($query, [$accId])->fetchAllAssociative();🤖 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/CharactersController.php` around lines 31 - 46, The character lookup in CharactersController is building SQL with a raw $accId from ACoreServices::I()->getAcoreAccountId(), which can be missing and lead to invalid queries. In the controller flow that prepares the characters list, guard for a missing account ID before executing the query, and if present, bind the account value as a parameter instead of interpolating it into the SQL string. Use the existing query/connection logic around getAcoreAccountId(), getCharacterEm()->getConnection(), and executeQuery() to keep the fix localized.src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php-15-25 (1)
15-25: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAdd a retention path for stored login IPs.
This stores website and in-game IP history indefinitely. IP addresses are sensitive personal data; add a retention setting and scheduled pruning before enabling this broadly.
Also applies to: 44-54, 94-104, 330-337
🤖 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 login IP storage in ConnectionLogger currently keeps sensitive address history indefinitely, so add a configurable retention policy and scheduled pruning before broad use. Update the table/record flow around the schema creation and the logging logic in ConnectionLogger to respect a retention setting, and add a cleanup routine that deletes entries older than the configured window. Use the existing ConnectionLogger methods and schema definition as the integration points, and make sure the pruning is wired into the plugin’s scheduled job path rather than leaving data to accumulate forever.src/acore-wp-plugin/src/Hooks/User/User.php-784-819 (1)
784-819: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove account lookup inside the guarded block.
Line 785 can initialize DB services before the
try, so a connection/config failure can breakprofile.phpadmin notices instead of silently skipping the badge.Proposed fix
$now = time(); - $accId = \ACore\Manager\ACoreServices::I()->getAcoreAccountId(); - if (!$accId) return; try { - $authConn = \ACore\Manager\ACoreServices::I()->getAccountEm()->getConnection(); - $charConn = \ACore\Manager\ACoreServices::I()->getCharacterEm()->getConnection(); + $services = \ACore\Manager\ACoreServices::I(); + $accId = $services->getAcoreAccountId(); + if (!$accId) return; + + $authConn = $services->getAccountEm()->getConnection(); + $charConn = $services->getCharacterEm()->getConnection();🤖 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 784 - 819, Move the account/service lookup into the protected badge-check path so a connection/config failure does not escape before error handling. In the User class method that builds the admin notice badge, keep the ACoreServices::I() calls and getAcoreAccountId/getConnection lookups inside the try or after the guard, and return early only after the database-backed checks have safely run. This ensures profile.php notices fail closed instead of breaking the page when DB services are unavailable.src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php-66-78 (1)
66-78: 🩺 Stability & Availability | 🟠 MajorCatch
\Throwablearound the SOAP password sync.
catch (\Exception $e)won’t intercept PHPErrors from service setup or SOAP calls, so the request can still fatal instead of taking the “website password unchanged” error path.Proposed fix
- } catch (\Exception $e) { + } catch (\Throwable $e) { $soapError = $e->getMessage(); }🤖 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 66 - 78, The SOAP password sync in PasswordHandler::handlePasswordChange only catches \Exception, so PHP Errors can still fatal before the “website password unchanged” fallback runs. Update the try/catch around ACoreServices::I()->getAccountSoap() and setAccountPassword() to catch \Throwable instead, and keep assigning the message into $soapError so the existing error path is used for both Exceptions and Errors.src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php-228-230 (1)
228-230: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAvoid plaintext GeoIP lookups Both lookup paths still send login IPs to
http://ip-api.comover HTTP (/jsonand/batch). ip-api’s free endpoints are HTTP-only, so switch to an HTTPS-capable provider/pro plan or make GeoIP an explicit admin opt-in with clear disclosure.🤖 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 lookup in ConnectionLogger still sends login IPs to ip-api over plain HTTP, so update the lookup path in the relevant lookup helper(s) used by ConnectionLogger to avoid transmitting data insecurely. Replace the current ip-api-based requests with an HTTPS-capable provider or gate GeoIP behind an explicit admin opt-in with clear disclosure, and make sure both the single-IP and batch lookup flows are handled consistently.src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php-582-590 (1)
582-590: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReset leaves the old thresholds in storage.
This handler removes every threshold input before submit, but
SettingsController::loadTools()only persists keys that are present in$_POST. That meansacore_name_unlock_thresholdsis never overwritten here, so the previous thresholds survive the “reset”.🤖 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 582 - 590, The reset handler for acore-name-unlock-reset is clearing the threshold rows in the UI, but that does not remove the stored acore_name_unlock_thresholds value because SettingsController::loadTools() only saves fields present in $_POST. Update the Tools.php click callback so the form submits an explicit empty value for the thresholds setting before calling submit, ensuring the reset path overwrites the old stored thresholds instead of leaving them behind.src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php-264-270 (1)
264-270: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse real buttons for the threshold actions.
These controls are rendered as
divs, so they are not keyboard-focusable and are not exposed as buttons to assistive tech. That makes threshold management unreachable for keyboard-only admins.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); $btnDel.append(`<span class="dashicons dashicons-trash"></span>`);Also applies to: 543-545
🤖 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 264 - 270, The threshold action controls in the AdminPanel Tools view are implemented as div-based pseudo-buttons, so they are not keyboard-accessible or announced properly to assistive tech. Update the Add/Reset controls in Tools.php (the threshold actions rendered near the acore-name-unlock-thresholds-add and acore-name-unlock-reset IDs, including the matching threshold block later in the file) to use real button elements while preserving the existing styling and click behavior.src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php-177-190 (1)
177-190: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist GeoIP as disabled when Security Logging is turned off.
If this page loads with Security Logging enabled and the user switches it off, the change handler only disables the GeoIP select. Disabled inputs are omitted from
$_POST, andSettingsController::loadTools()only stores posted keys, so the previousacore_geoip_lookupvalue survives and can silently come back on later.Proposed fix
$('`#acore_security_logging`').on('change', function(){ var on = $(this).val() === '1'; + var $form = $(this).closest('form'); + $form.find('input[type="hidden"][name="acore_geoip_lookup"]').remove(); + $('`#acore-geoip-wrap`').toggleClass('acore-days-inactive-disabled', !on) .attr('title', on ? '' : 'Security Logging must be enabled'); - $('`#acore_geoip_lookup`').prop('disabled', !on); + $('`#acore_geoip_lookup`').prop('disabled', !on); + + if (!on) { + $('`#acore_geoip_lookup`').val('0'); + $('<input>', { + type: 'hidden', + name: 'acore_geoip_lookup', + value: '0' + }).appendTo($form); + } });Also applies to: 336-342
🤖 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 177 - 190, Persist acore_geoip_lookup as disabled whenever Security Logging is turned off, because the GeoIP select is disabled and omitted from $_POST so SettingsController::loadTools can keep the old value. Update the Tools page behavior around the acore_geoip_lookup field (including the change-handler logic that disables it) so a disabled state still submits an explicit 0, or otherwise ensure the settings payload always includes acore_geoip_lookup when acore_security_logging is not enabled. Keep the UI state and the stored option in sync so the disabled setting cannot silently re-enable later.src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php-193-199 (1)
193-199: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow Old Passwordsis currently a no-op.This toggle never changes password behavior:
acore_process_security_password()still rejects reused passwords unconditionally, so enabling the setting has no effect.Proposed fix
- if (acore_password_is_reused($user->ID, $newPass)) { + if (\ACore\Manager\Opts::I()->acore_allow_old_passwords !== '1' + && acore_password_is_reused($user->ID, $newPass)) { acore_pw_set_message($user->ID, 'error', __('This password has been used before. Please choose a different one.', 'acore-wp-plugin')); wp_redirect($security_url); exit; }🤖 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 193 - 199, The “Allow Old Passwords” setting is currently ignored by the password-change flow, so wire the toggle through the password validation path. Update acore_process_security_password() to read Opts::I()->acore_allow_old_passwords and skip the reused-password rejection when the option is enabled, while preserving the existing check when it is disabled. Use the existing acore_allow_old_passwords option and the password validation logic in acore_process_security_password() so the admin toggle actually changes behavior.src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php-215-235 (1)
215-235: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon't infer success from the word
"mail".This branch only removes the card when the raw SOAP response happens to contain
"mail". Any wording or localization change will turn a successful restore into an apparent failure and leave the UI out of sync. Please return a structured REST payload here (for examplesuccess/message) and branch on that instead.🤖 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 215 - 235, The restore success handling in the ItemRestorationPage flow is currently inferring success from a raw response string containing “mail”, which is brittle. Update the response handling in the restore promise chain to use a structured REST payload with explicit success/message fields instead of string matching, and branch in the existing success callback based on that shape. Keep the card removal, renumbering, and empty-state logic in the same success path, and use the existing error path for non-success responses.src/acore-wp-plugin/src/Manager/ACoreServices.php-408-418 (1)
408-418: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't throw uncaught
InvalidArgumentExceptionout of the restore-list path.
ToolsApi::ItemRestoreList()calls this method directly from a REST callback, so these new exceptions will turn a bad or forgedcguidinto a 500 instead of a clean 4xx response. Convert this failure into a route-levelWP_Error, or stop throwing here and return a sentinel the REST layer can map explicitly.🤖 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/Manager/ACoreServices.php` around lines 408 - 418, ACoreServices::restore character ownership validation currently throws InvalidArgumentException from the restore-list path, which can bubble through ToolsApi::ItemRestoreList into a 500. Update the flow so this failure is handled at the REST boundary: either have ACoreServices return a clear sentinel/error state instead of throwing, or catch the exception in ToolsApi::ItemRestoreList and convert it into a WP_Error with an appropriate 4xx status. Keep the behavior for valid restores unchanged and ensure the new handling covers both invalid cguid input and missing character ownership.src/acore-wp-plugin/web/assets/css/main.css-690-695 (1)
690-695: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the restore grid responsive to the actual content width.
With a fixed 460px sidebar, the content column is often far too narrow for four cards that each need an 80px icon plus padding, so this grid starts overflowing well before the 800px breakpoint. An adaptive definition avoids that.
Suggested CSS
.item-restore-grid { display: grid; - grid-template-columns: repeat(4, 1fr); + grid-template-columns: repeat(auto-fit, minmax(140px, 1fr)); gap: 12px; }🤖 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/main.css` around lines 690 - 695, The fixed four-column definition in item-restore-grid is causing overflow in the narrow content column. Update the grid in main.css to adapt based on available width instead of always using four equal columns, and keep the change scoped to the .item-restore-grid rule so the cards wrap naturally before the sidebar-constrained layout breaks.src/acore-wp-plugin/web/assets/mail-return/mail-return.js-2-18 (1)
2-18: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAbort or ignore stale mail-list requests.
Rapidly clicking two characters can let the slower response render last, leaving one character highlighted while another character's mails and
Returnbuttons are on screen. That makes the destructive action target the wrong mail set. Abort the previous XHR or discard responses whosecharGuidis no longer active.Suggested fix
+ var currentRequest = null; + var activeCharGuid = null; + jQuery("`#acore-characters-mail`").on("click", ".acore-char-card", function () { var card = jQuery(this); var charGuid = card.data("char-guid"); @@ + activeCharGuid = charGuid; + if (currentRequest) { + currentRequest.abort(); + } + - jQuery.ajax({ + currentRequest = jQuery.ajax({ success: function (mails) { + if (activeCharGuid !== charGuid) { + return; + } loading.hide(); @@ }, error: function (xhr, status, error) { + if (status === "abort") { + return; + } loading.hide(); emptyMsg.text("Failed to load mails."); emptyWrap.show(); console.error("Failed to load mails:", error); + }, + complete: function () { + currentRequest = null; }Also applies to: 25-34, 65-149, 180-183
🤖 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, The mail list loading flow in mail-return.js can render stale results when multiple character cards are clicked quickly, so update the character-switch request handling to ignore or cancel outdated responses. Use the click handler on jQuery("`#acore-characters-mail`") / ".acore-char-card" and the mail-loading path that populates "`#mail-return-items`" to track the active charGuid, abort any previous XHR, or discard any callback whose charGuid no longer matches the current active card before rendering mails and Return buttons. Ensure the response-handling logic only updates the UI for the most recently selected character.
🟡 Minor comments (8)
src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php-314-328 (1)
314-328: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the visible module list after auto refresh too.
The auto refresh persists fresh modules, but skips repainting the list/timestamp, so the panel can keep showing stale modules or “No modules loaded” until a manual refresh.
Proposed fix
- if (!opts.auto) { - var $list = $('`#acore-modules-list`').empty(); - if (newModules.length) { - newModules.forEach(function(m){ - $list.append($('<span>').addClass('acore-module-tag').text(m)); - }); - } else { - $list.append($('<span>').addClass('acore-modules-empty').text('No modules found.')); - } - var d = new Date(res.refreshed * 1000); - $('`#acore-modules-refreshed`').html('Last refreshed:<br>' + d.toLocaleString()); + var $list = $('`#acore-modules-list`').empty(); + if (newModules.length) { + newModules.forEach(function(m){ + $list.append($('<span>').addClass('acore-module-tag').text(m)); + }); + } else { + $list.append($('<span>').addClass('acore-modules-empty').text('No modules found.')); } + var d = new Date(res.refreshed * 1000); + $('`#acore-modules-refreshed`').html('Last refreshed:<br>' + d.toLocaleString());🤖 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 314 - 328, The auto-refresh path in RealmSettings should update the visible module list and refreshed timestamp the same way the manual refresh path does. After the modules are persisted in the refresh handler, make sure the UI repaint logic that updates `#acore-modules-list` and `#acore-modules-refreshed` runs even when opts.auto is true, using the existing newModules and res.refreshed values; keep renderAutoCheck(diffReferenced(...)) and knownModules assignment in sync afterward.src/acore-wp-plugin/src/Components/UserPanel/UserController.php-217-221 (1)
217-221: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale
getModel()return contract.The docblock still says
UserModel, but Line 221 returns the controller itself. Either update the docblock to@return self/UserControlleror restore the intended model object.🤖 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 217 - 221, The getModel() contract is stale because UserController::getModel() returns the controller instance, not a UserModel. Update the docblock on getModel() to match the actual return type (self/UserController) or change the method to return the intended model object if that was the original design; make sure the signature and documentation are consistent.Source: Linters/SAST tools
src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php-28-29 (1)
28-29: 🎯 Functional Correctness | 🟡 MinorUse
wp_date()for these timestampssrc/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php:28-29renders user-facing ban/mute end times, sodate()can drift from the site timezone. Switch both helpers towp_date().🤖 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 28 - 29, The timestamp helpers in CharactersView currently use date(), which can ignore the site timezone for user-facing ban/mute end times. Update the formatDate and formatTime methods to use wp_date() instead, preserving the existing formats while relying on WordPress timezone handling.src/acore-wp-plugin/src/Hooks/User/User.php-877-877 (1)
877-877: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t label IPv6 addresses as IPv4.
acore_resolve_client_ip()can return IPv6, so the heading should say “Your IP” instead.Proposed fix
- <h2 class="acore-conn-heading"><span><?php _e('Recent Connections', 'acore-wp-plugin'); ?></span><span class="acore-conn-myip"><?php _e('Your IPv4:', 'acore-wp-plugin'); ?> <?= esc_html($myIp) ?></span></h2> + <h2 class="acore-conn-heading"><span><?php _e('Recent Connections', 'acore-wp-plugin'); ?></span><span class="acore-conn-myip"><?php _e('Your IP:', 'acore-wp-plugin'); ?> <?= esc_html($myIp) ?></span></h2>🤖 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` at line 877, The heading text in the User rendering block is incorrectly hardcoded as IPv4 even though acore_resolve_client_ip() may return IPv6. Update the label in the User.php view inside the Recent Connections header to say “Your IP” instead of “Your IPv4,” keeping the existing $myIp output and related escaping unchanged.src/acore-wp-plugin/src/Utils/AcoreCharColors.php-84-93 (1)
84-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a neutral fallback for unknown factions.
Line 87 currently renders any non-alliance race as Horde red, so unknown race IDs get Horde styling. Line 88 also assigns
$fDarkbut never uses it.Proposed fix
- $fLight = $faction === 'alliance' ? '`#3FACF4`' : '`#FF653D`'; - $fDark = $faction === 'alliance' ? '`#3FACF4`' : '`#FF653D`'; + $fLight = $faction === 'alliance' + ? '`#3FACF4`' + : ($faction === 'horde' ? '`#FF653D`' : self::FALLBACK_LIGHT);🤖 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/Utils/AcoreCharColors.php` around lines 84 - 93, The rowStyle method in AcoreCharColors is treating every non-alliance race as Horde, so unknown race IDs get the wrong styling; update the faction-color fallback in rowStyle to use a neutral value when RACE_FACTION returns unknown, and keep alliance/Horde colors only for explicit matches. Also remove or stop computing the unused $fDark variable in rowStyle so the logic stays clean and consistent.Source: Linters/SAST tools
src/acore-wp-plugin/src/Components/UserPanel/Pages/RafProgressPage.php-11-20 (1)
11-20: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the declared text-domain property here.
Optsexposesacore_page_alias, notpage_alias, so these new_e()calls resolve against a null/empty domain and the new copy will not be translatable.Proposed fix
- <h1><?php _e('Recruit a Friend', Opts::I()->page_alias); ?></h1> - <p class="acore-raf-intro"><?php _e('Recruit your friends, help them level up and earn unique rewards.', Opts::I()->page_alias); ?></p> + <h1><?php _e('Recruit a Friend', Opts::I()->acore_page_alias); ?></h1> + <p class="acore-raf-intro"><?php _e('Recruit your friends, help them level up and earn unique rewards.', Opts::I()->acore_page_alias); ?></p>🤖 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/RafProgressPage.php` around lines 11 - 20, The new localization calls in RafProgressPage are using the wrong text-domain property, which breaks translation lookups. Update the `_e()` calls in this view to use `Opts::I()->acore_page_alias` instead of `Opts::I()->page_alias`, keeping the existing copy and structure intact so the strings are registered under the declared domain.src/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.php-82-90 (1)
82-90: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStart the mail panel hidden.
The markup says this column stays hidden until mails are loaded, but
#mail-return-contentrenders immediately, so users land on an empty card before selecting a character.🤖 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/MailReturnMenu/MailReturnView.php` around lines 82 - 90, The mail return panel is rendering visible immediately even though it should stay hidden until mail data is loaded. Update the markup in MailReturnView so the `#mail-return-content` container starts hidden by default and is only shown by the existing JS once the mails are ready; keep the change scoped to the mail panel structure around the mail list and heading elements.src/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.php-25-36 (1)
25-36: 🎯 Functional Correctness | 🟡 MinorDefine
window.whTooltipsbefore loadingpower.js
The currentconst whTooltips = ...runs after the CDN script and does not expose a global config object. Move this to an inline-before assignment (or enqueue awindow.whTooltipsblock) ahead ofpower.js.🤖 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/MailReturnMenu/MailReturnView.php` around lines 25 - 36, Define the tooltip config as a global before the Power.js CDN script loads, because the current MailReturnView setup creates a local const after enqueueing power.js and it won’t be picked up. Update the logic in MailReturnView so the whTooltips assignment is emitted as window.whTooltips (or an inline script inserted before wp_enqueue_script for power.js) and keep the existing options intact. Use the unique symbols wp_enqueue_script('power-js') and the whTooltips block to place the fix in the right spot.
🤖 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/MailReturnMenu/MailReturnView.php`:
- Around line 25-32: Remove the remote power.js enqueue from MailReturnView and
avoid exposing mailReturnData.nonce to any third-party script on this privileged
page. Update the MailReturnView render path to use a local vendored/pinned asset
or move the needed behavior server-side, and keep the wp_localize_script data
scoped only to trusted code in acore-mail-return-js.
In `@src/acore-wp-plugin/src/Components/Tools/ToolsApi.php`:
- Around line 15-20: The restore path in ToolsApi is only checking
currentAccountOwnsCharacterName($cname), which still allows a guessed recovery
item id to be used with an owned character name. Update the authorization logic
around the $item validation so the restore item is verified against the caller’s
account by joining the recovery item back to characters.account before any SOAP
restore command is executed. Keep the existing item-id validation, but add the
ownership check for the specific restore item itself in the same flow that uses
$item and $cname.
In `@src/acore-wp-plugin/src/Hooks/User/User.php`:
- Line 822: The date formatter in User::getUserData (the $fmtDate assignment)
uses an arrow function, which is incompatible with the declared PHP 7.1 runtime.
Replace the fn($ts) => ... expression with a standard anonymous function that
returns the same formatted date string, keeping the existing behavior intact.
---
Major comments:
In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php`:
- Around line 272-280: The checkSoap status request is missing the REST nonce,
so the `server-info` call can be treated as unauthenticated for logged-in
admins. Update the `checkSoap` function in `RealmSettings.php` so the
`$.get(restBase + 'server-info', ...)` request includes `X-WP-Nonce` the same
way the other REST calls do, using the existing nonce value already available in
this admin page. Keep the existing success/fail/always behavior in
`setSoapStatus` and the `manual` button state handling unchanged.
In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php`:
- Around line 582-590: The reset handler for acore-name-unlock-reset is clearing
the threshold rows in the UI, but that does not remove the stored
acore_name_unlock_thresholds value because SettingsController::loadTools() only
saves fields present in $_POST. Update the Tools.php click callback so the form
submits an explicit empty value for the thresholds setting before calling
submit, ensuring the reset path overwrites the old stored thresholds instead of
leaving them behind.
- Around line 264-270: The threshold action controls in the AdminPanel Tools
view are implemented as div-based pseudo-buttons, so they are not
keyboard-accessible or announced properly to assistive tech. Update the
Add/Reset controls in Tools.php (the threshold actions rendered near the
acore-name-unlock-thresholds-add and acore-name-unlock-reset IDs, including the
matching threshold block later in the file) to use real button elements while
preserving the existing styling and click behavior.
- Around line 177-190: Persist acore_geoip_lookup as disabled whenever Security
Logging is turned off, because the GeoIP select is disabled and omitted from
$_POST so SettingsController::loadTools can keep the old value. Update the Tools
page behavior around the acore_geoip_lookup field (including the change-handler
logic that disables it) so a disabled state still submits an explicit 0, or
otherwise ensure the settings payload always includes acore_geoip_lookup when
acore_security_logging is not enabled. Keep the UI state and the stored option
in sync so the disabled setting cannot silently re-enable later.
- Around line 193-199: The “Allow Old Passwords” setting is currently ignored by
the password-change flow, so wire the toggle through the password validation
path. Update acore_process_security_password() to read
Opts::I()->acore_allow_old_passwords and skip the reused-password rejection when
the option is enabled, while preserving the existing check when it is disabled.
Use the existing acore_allow_old_passwords option and the password validation
logic in acore_process_security_password() so the admin toggle actually changes
behavior.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.php`:
- Around line 56-61: The account-ban lookup in CharactersController::...
currently excludes permanent bans because the query only accepts unbandate = 0
or future timestamps. Update the account_banned query used for $accBanRow so it
also treats unbandate = bandate as a permanent ban, matching the character-ban
logic. Keep the existing active and latest-row behavior intact while broadening
the WHERE clause to include this permanent-ban case.
- Around line 31-46: The character lookup in CharactersController is building
SQL with a raw $accId from ACoreServices::I()->getAcoreAccountId(), which can be
missing and lead to invalid queries. In the controller flow that prepares the
characters list, guard for a missing account ID before executing the query, and
if present, bind the account value as a parameter instead of interpolating it
into the SQL string. Use the existing query/connection logic around
getAcoreAccountId(), getCharacterEm()->getConnection(), and executeQuery() to
keep the fix localized.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersMenu.php`:
- Around line 35-39: The CharactersMenu badge query is missing the permanent-ban
condition used elsewhere, so accounts banned with matching unbandate and bandate
won’t be marked banned. Update the query in CharactersMenu::executeQuery usage
to mirror the existing permanent-ban predicate by adding the unbandate = bandate
check alongside the current active and unbandate timestamp logic, keeping the
badge state consistent for all permanent bans.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php`:
- Around line 40-42: The account-ban rendering logic in CharactersView treats
only unbandate = 0 as permanent, so equal bandate/unbandate rows still show as a
temporary ban with 0 seconds remaining. Update the ban-state checks around
$isAccountBanned, $accBanPerma, and $accBanRemaining so that rows where
unbandate equals bandate are also classified as permanent, and ensure the UI
path that displays “Banned” is used for that case instead of the remaining-time
display.
In `@src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php`:
- Around line 200-208: ServerInfoApi::serverInfo() may return an Exception or
other non-string value, so the SOAP error detection loop must guard before
calling stripos(). In the server info handling block, normalize or branch on the
result type first, then only run the errorPatterns matching for string
responses; for non-string failures, log them and return the intended 503 path
directly. Use the existing ServerInfoApi::serverInfo, $result, and
$errorPatterns block to locate the fix.
- Around line 447-456: The fresh-token branch in ServerInfoApi’s in-game 2FA
removal flow is missing the same 5-attempt limiter used by verify-website-2fa,
so repeated wrong TOTP submissions are not rate-limited. Update the direct
verification path around acore_wp2fa_code_is_valid to reuse the existing
attempt-tracking transient logic (increment on failure, block after 5, reset on
success) with the same user-scoped key pattern used elsewhere in ServerInfoApi.
Keep the authorization flow unchanged, but ensure both the unlock-based and
fresh-token paths enforce the same retry protection before returning
wrong_token.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php`:
- Around line 215-235: The restore success handling in the ItemRestorationPage
flow is currently inferring success from a raw response string containing
“mail”, which is brittle. Update the response handling in the restore promise
chain to use a structured REST payload with explicit success/message fields
instead of string matching, and branch in the existing success callback based on
that shape. Keep the card removal, renumbering, and empty-state logic in the
same success path, and use the existing error path for non-success responses.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php`:
- Around line 220-243: The WP2FA management panel is still being rendered into
the DOM in the gated branch, so the unlock is only cosmetic and can be bypassed
by inspecting hidden HTML. Update the SecurityPage rendering logic so $wp2faHtml
is emitted only when $twofaUnlocked is true, and keep the gated branch from
outputting the protected panel until a server-side unlock has occurred. After a
successful TOTP verification in the related unlock flow, reload the page so the
transient-controlled $twofaUnlocked state can render the panel, and apply the
same server-side gating pattern to the email-gate branch.
In `@src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php`:
- Around line 15-25: The login IP storage in ConnectionLogger currently keeps
sensitive address history indefinitely, so add a configurable retention policy
and scheduled pruning before broad use. Update the table/record flow around the
schema creation and the logging logic in ConnectionLogger to respect a retention
setting, and add a cleanup routine that deletes entries older than the
configured window. Use the existing ConnectionLogger methods and schema
definition as the integration points, and make sure the pruning is wired into
the plugin’s scheduled job path rather than leaving data to accumulate forever.
- Around line 228-230: The GeoIP lookup in ConnectionLogger still sends login
IPs to ip-api over plain HTTP, so update the lookup path in the relevant lookup
helper(s) used by ConnectionLogger to avoid transmitting data insecurely.
Replace the current ip-api-based requests with an HTTPS-capable provider or gate
GeoIP behind an explicit admin opt-in with clear disclosure, and make sure both
the single-IP and batch lookup flows are handled consistently.
In `@src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php`:
- Around line 66-78: The SOAP password sync in
PasswordHandler::handlePasswordChange only catches \Exception, so PHP Errors can
still fatal before the “website password unchanged” fallback runs. Update the
try/catch around ACoreServices::I()->getAccountSoap() and setAccountPassword()
to catch \Throwable instead, and keep assigning the message into $soapError so
the existing error path is used for both Exceptions and Errors.
In `@src/acore-wp-plugin/src/Hooks/User/User.php`:
- Around line 784-819: Move the account/service lookup into the protected
badge-check path so a connection/config failure does not escape before error
handling. In the User class method that builds the admin notice badge, keep the
ACoreServices::I() calls and getAcoreAccountId/getConnection lookups inside the
try or after the guard, and return early only after the database-backed checks
have safely run. This ensures profile.php notices fail closed instead of
breaking the page when DB services are unavailable.
In `@src/acore-wp-plugin/src/Manager/ACoreServices.php`:
- Around line 408-418: ACoreServices::restore character ownership validation
currently throws InvalidArgumentException from the restore-list path, which can
bubble through ToolsApi::ItemRestoreList into a 500. Update the flow so this
failure is handled at the REST boundary: either have ACoreServices return a
clear sentinel/error state instead of throwing, or catch the exception in
ToolsApi::ItemRestoreList and convert it into a WP_Error with an appropriate 4xx
status. Keep the behavior for valid restores unchanged and ensure the new
handling covers both invalid cguid input and missing character ownership.
In `@src/acore-wp-plugin/web/assets/css/main.css`:
- Around line 690-695: The fixed four-column definition in item-restore-grid is
causing overflow in the narrow content column. Update the grid in main.css to
adapt based on available width instead of always using four equal columns, and
keep the change scoped to the .item-restore-grid rule so the cards wrap
naturally before the sidebar-constrained layout breaks.
In `@src/acore-wp-plugin/web/assets/mail-return/mail-return.js`:
- Around line 2-18: The mail list loading flow in mail-return.js can render
stale results when multiple character cards are clicked quickly, so update the
character-switch request handling to ignore or cancel outdated responses. Use
the click handler on jQuery("`#acore-characters-mail`") / ".acore-char-card" and
the mail-loading path that populates "`#mail-return-items`" to track the active
charGuid, abort any previous XHR, or discard any callback whose charGuid no
longer matches the current active card before rendering mails and Return
buttons. Ensure the response-handling logic only updates the UI for the most
recently selected character.
---
Minor comments:
In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php`:
- Around line 314-328: The auto-refresh path in RealmSettings should update the
visible module list and refreshed timestamp the same way the manual refresh path
does. After the modules are persisted in the refresh handler, make sure the UI
repaint logic that updates `#acore-modules-list` and `#acore-modules-refreshed` runs
even when opts.auto is true, using the existing newModules and res.refreshed
values; keep renderAutoCheck(diffReferenced(...)) and knownModules assignment in
sync afterward.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php`:
- Around line 28-29: The timestamp helpers in CharactersView currently use
date(), which can ignore the site timezone for user-facing ban/mute end times.
Update the formatDate and formatTime methods to use wp_date() instead,
preserving the existing formats while relying on WordPress timezone handling.
In `@src/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.php`:
- Around line 82-90: The mail return panel is rendering visible immediately even
though it should stay hidden until mail data is loaded. Update the markup in
MailReturnView so the `#mail-return-content` container starts hidden by default
and is only shown by the existing JS once the mails are ready; keep the change
scoped to the mail panel structure around the mail list and heading elements.
- Around line 25-36: Define the tooltip config as a global before the Power.js
CDN script loads, because the current MailReturnView setup creates a local const
after enqueueing power.js and it won’t be picked up. Update the logic in
MailReturnView so the whTooltips assignment is emitted as window.whTooltips (or
an inline script inserted before wp_enqueue_script for power.js) and keep the
existing options intact. Use the unique symbols wp_enqueue_script('power-js')
and the whTooltips block to place the fix in the right spot.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/RafProgressPage.php`:
- Around line 11-20: The new localization calls in RafProgressPage are using the
wrong text-domain property, which breaks translation lookups. Update the `_e()`
calls in this view to use `Opts::I()->acore_page_alias` instead of
`Opts::I()->page_alias`, keeping the existing copy and structure intact so the
strings are registered under the declared domain.
In `@src/acore-wp-plugin/src/Components/UserPanel/UserController.php`:
- Around line 217-221: The getModel() contract is stale because
UserController::getModel() returns the controller instance, not a UserModel.
Update the docblock on getModel() to match the actual return type
(self/UserController) or change the method to return the intended model object
if that was the original design; make sure the signature and documentation are
consistent.
In `@src/acore-wp-plugin/src/Hooks/User/User.php`:
- Line 877: The heading text in the User rendering block is incorrectly
hardcoded as IPv4 even though acore_resolve_client_ip() may return IPv6. Update
the label in the User.php view inside the Recent Connections header to say “Your
IP” instead of “Your IPv4,” keeping the existing $myIp output and related
escaping unchanged.
In `@src/acore-wp-plugin/src/Utils/AcoreCharColors.php`:
- Around line 84-93: The rowStyle method in AcoreCharColors is treating every
non-alliance race as Horde, so unknown race IDs get the wrong styling; update
the faction-color fallback in rowStyle to use a neutral value when RACE_FACTION
returns unknown, and keep alliance/Horde colors only for explicit matches. Also
remove or stop computing the unused $fDark variable in rowStyle so the logic
stays clean and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
(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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/CharactersController.php`:
- Around line 48-50: The ordering in CharactersController::getCharacters
currently uses COALESCE(c.`order`, c.`guid`), which can interleave null-order
characters with saved-order rows; update the ORDER BY logic so rows with a real
`order` value are sorted together first, and only null-order rows fall back to
GUID-based ordering afterward. Use the query in CharactersController as the fix
point and adjust the ORDER BY expression to separate the two groups without
mixing the sort domains.
🪄 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: c9589379-f0ec-49ab-8c73-6cc68e41a416
📒 Files selected for processing (13)
src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.phpsrc/acore-wp-plugin/src/Components/AdminPanel/SettingsController.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/MailReturnView.phpsrc/acore-wp-plugin/src/Components/Tools/ToolsApi.phpsrc/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.phpsrc/acore-wp-plugin/src/Hooks/User/User.phpsrc/acore-wp-plugin/src/Hooks/Various/DarkMode.phpsrc/acore-wp-plugin/src/Manager/ACoreServices.phpsrc/acore-wp-plugin/src/Utils/AcoreCharColors.phpsrc/acore-wp-plugin/web/assets/css/main.css
🚧 Files skipped from review as they are similar to previous changes (11)
- src/acore-wp-plugin/src/Utils/AcoreCharColors.php
- src/acore-wp-plugin/src/Components/AdminPanel/SettingsController.php
- src/acore-wp-plugin/src/Components/Tools/ToolsApi.php
- src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php
- src/acore-wp-plugin/web/assets/css/main.css
- src/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.php
- src/acore-wp-plugin/src/Components/CharactersMenu/CharactersMenu.php
- src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php
- src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php
- src/acore-wp-plugin/src/Hooks/Various/DarkMode.php
- src/acore-wp-plugin/src/Hooks/User/User.php
This was referenced Jun 24, 2026
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
Also opening this so it can be sorted for review by Rabbit, so I can also do 1 last PR which implements PDUMP (with a random GUID and other info to not btraced to the real player).
Account Muted, Account Banned and Characters Banned (it got re-order to properly match the character order instad of name)
Character tab now displays if account is banned or if it's muted (banned > muted)
Banned characters
Muted (not serving yet) + Banned Characters
Muted (serving) + Banned Characters
Muted (Serving) + Banned Account + Banned Characters
"Banned" = Perma Banned
Summary by CodeRabbit
New Features
Bug Fixes