Multiple Changes to Acore-CMS user and admin experience.#197
Closed
TheSCREWEDSoftware wants to merge 48 commits into
Closed
Multiple Changes to Acore-CMS user and admin experience.#197TheSCREWEDSoftware wants to merge 48 commits into
TheSCREWEDSoftware wants to merge 48 commits into
Conversation
Moved from Profile to Secruity - Password Change - 2FA Profile page also show the latest connections at the bottom The most recent 20 connections Security tab will show up to 20 and you can have view to sell all history
In the scenarios where the user tried to change their current password to older one (for the first time) it would log them out, never was an issue before as we didn't prevent it, the previous implementation has been reworked to make the entire check happens before the page load
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php-18-19 (1)
18-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd CSRF protection to the settings save flow.
Line 18 defines a state-changing admin form without a nonce, and Line 322 posts it directly. A forged request can change SOAP/DB credentials.
Suggested fix
- <form name="form-acore-settings" method="post" action=""> + <form name="form-acore-settings" method="post" action=""> + <?php wp_nonce_field('acore_realm_settings_save', 'acore_realm_settings_nonce'); ?>$('form[name="form-acore-settings"]').on('submit', function(e){(Then verify server-side with
check_admin_referer('acore_realm_settings_save', 'acore_realm_settings_nonce')before persisting.)Also applies to: 318-323
🤖 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 18 - 19, The form with name "form-acore-settings" lacks CSRF protection, allowing forged requests to modify sensitive SOAP/DB credentials. Add a WordPress nonce field inside the form using wp_nonce_field with the action 'acore_realm_settings_save' and field name 'acore_realm_settings_nonce'. Then locate the form processing code that handles the form submission (around lines 318-323) and add server-side verification at the beginning using check_admin_referer('acore_realm_settings_save', 'acore_realm_settings_nonce') before any credential data is persisted to ensure the request is legitimate.src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php-27-133 (1)
27-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape option values before injecting into input attributes.
Lines like Line 27/43/55/77/103/129 print raw option values into
value="". Quotes/special chars can break the form and create stored XSS exposure in admin.Suggested fix pattern
-<td><input type="text" name="acore_realm_alias" value="<?= Opts::I()->acore_realm_alias; ?>" size="20" placeholder="AzerothCore"></td> +<td><input type="text" name="acore_realm_alias" value="<?= esc_attr(Opts::I()->acore_realm_alias); ?>" size="20" placeholder="AzerothCore"></td> -<td><input type="password" name="acore_soap_pass" value="<?= Opts::I()->acore_soap_pass; ?>" size="20"></td> +<td><input type="password" name="acore_soap_pass" value="<?= esc_attr(Opts::I()->acore_soap_pass); ?>" size="20"></td>Apply the same
esc_attr(...)pattern to all inputvalueattributes in this form.🤖 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 27 - 133, All input value attributes in the RealmSettings.php form are directly injecting raw option values from Opts::I() calls without escaping, creating a stored XSS vulnerability. Wrap each value attribute with the esc_attr() function to properly escape the output. This applies to all input fields including acore_realm_alias, acore_soap_host, acore_soap_port, acore_soap_user, acore_soap_pass, acore_db_auth_host, acore_db_auth_port, acore_db_auth_user, acore_db_auth_pass, acore_db_auth_name, acore_db_char_host, acore_db_char_port, acore_db_char_user, acore_db_char_pass, acore_db_char_name, acore_db_world_host, acore_db_world_port, acore_db_world_user, acore_db_world_pass, and acore_db_world_name. Change each instance from value="<?= Opts::I()->optionName; ?>" to value="<?= esc_attr(Opts::I()->optionName); ?>".src/acore-wp-plugin/web/assets/css/main.css-114-116 (1)
114-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t use
display: nonefor interactive radios in the expansion selector.Line 115 removes the radio controls from keyboard and assistive-tech navigation. This breaks non-mouse interaction for selecting expansions.
Suggested fix
.acore-expansion-option input[type="radio"] { - display: none; + position: absolute; + width: 1px; + height: 1px; + margin: -1px; + padding: 0; + border: 0; + overflow: hidden; + clip: rect(0 0 0 0); + clip-path: inset(50%); + white-space: nowrap; }🤖 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 114 - 116, The expansion selector radio controls are currently hidden using display: none, which removes them from keyboard navigation and assistive technology trees, breaking accessibility for non-mouse users. Replace the display: none property on the radio input elements in the expansion selector with an accessible hiding technique such as using a sr-only class pattern, position absolute with negative left offset, or clip method that visually hides the element while keeping it in the accessibility tree. This ensures keyboard users and screen reader users can still interact with and navigate the expansion selection controls.src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php-26-37 (1)
26-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake character cards keyboard-activatable.
role="button"alone does not make the row focusable or handle Enter/Space, so keyboard users cannot select a character. Use a real button so the existing click listener still works.♿ Proposed fix
- <div class="acore-char-row acore-char-card" + <button type="button" + class="acore-char-row acore-char-card" data-char-guid="<?= intval($char['guid']) ?>" data-char-name="<?= esc_attr($char['name']) ?>" - style="<?= esc_attr($clsStyle) ?>" - role="button"> + style="<?= esc_attr($clsStyle) ?>"> <span class="acore-char-name"><?= esc_html($char['name']) ?></span> <span class="acore-char-meta"> <span class="acore-level" data-exp="<?= AcoreCharColors::expansionSlug(intval($char['level'])) ?>" title="<?= esc_attr(AcoreCharColors::expansionLabel(intval($char['level']))) ?>">Level <?= intval($char['level']) ?></span> <img class="race-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getRaceName(intval($char['race']))) ?>" src="<?= ACORE_URL_PLG . 'web/assets/race/' . intval($char['race']) . (intval($char['gender']) == 0 ? 'm' : 'f') . '.webp' ?>"> <img class="class-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getClassName(intval($char['class']))) ?>" src="<?= ACORE_URL_PLG . 'web/assets/class/' . intval($char['class']) . '.webp' ?>"> </span> - </div> + </button>Also applies to: 94-101
🤖 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 26 - 37, The character card container uses a div element with role="button" which is not keyboard-accessible. Replace the div element that contains the character card (with class "acore-char-row acore-char-card" and data attributes for char-guid and char-name) with a proper button element instead. Keep all existing data attributes, styling, and child content intact, and apply the same change to the other occurrence mentioned at lines 94-101. This will automatically provide keyboard focus and Enter/Space key handling while maintaining compatibility with the existing click listeners.src/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.php-53-61 (1)
53-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a real button for selectable character cards.
The mail list can only be loaded by clicking this
div; keyboard users cannot focus or activate it. Abuttonpreserves the existing delegated click handler while adding keyboard activation.♿ Proposed fix
- <div class="acore-char-row acore-char-card" data-char-guid="<?= intval($char['guid']) ?>" style="<?= esc_attr($clsStyle) ?>"> + <button type="button" class="acore-char-row acore-char-card" data-char-guid="<?= intval($char['guid']) ?>" style="<?= esc_attr($clsStyle) ?>"> <span class="acore-char-name"><?= esc_html($char['name']) ?></span> <span class="acore-char-meta"> <span class="acore-level" data-exp="<?= AcoreCharColors::expansionSlug(intval($char['level'])) ?>" title="<?= esc_attr(AcoreCharColors::expansionLabel(intval($char['level']))) ?>">Level <?= intval($char['level']) ?></span> <img class="race-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getRaceName(intval($char['race']))) ?>" src="<?= ACORE_URL_PLG . 'web/assets/race/' . intval($char['race']) . (intval($char['gender']) == 0 ? 'm' : 'f') . '.webp' ?>"> <img class="class-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getClassName(intval($char['class']))) ?>" src="<?= ACORE_URL_PLG . 'web/assets/class/' . intval($char['class']) . '.webp' ?>"> </span> - </div> + </button>🤖 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 53 - 61, The character card element with class acore-char-card is currently a div, which cannot be focused or activated by keyboard users. Replace this div with a button element to preserve the existing delegated click handler while adding keyboard accessibility. The button should maintain all the same attributes (data-char-guid, style, and inner content with spans like acore-char-name and acore-char-meta) as the current div implementation.src/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.php-16-27 (1)
16-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect character-order POST actions with a nonce.
Both save and reset are accepted solely from POST fields, so a third-party page can submit a logged-in user’s browser and reset/reorder characters. Verify a nonce before branching, and add the matching field in
CharactersView.🛡️ Proposed fix
public function loadHome() { - if ($_SERVER["REQUEST_METHOD"] == "POST") { + if ($_SERVER["REQUEST_METHOD"] == "POST") { + check_admin_referer('acore_character_order', 'acore_character_order_nonce'); + if (isset($_POST["acore_reset_order"])) { $this->resetCharacterOrder();And add the matching field inside the form in
CharactersView:<form action="" method="POST" novalidate="novalidate"> + <?php wp_nonce_field('acore_character_order', 'acore_character_order_nonce'); ?>🤖 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 16 - 27, The POST request handling logic in the CharactersController.php file (specifically the REQUEST_METHOD check and the subsequent branches for acore_reset_order with resetCharacterOrder() and saveCharacterOrder() calls) lacks nonce verification, making it vulnerable to CSRF attacks. Add nonce verification immediately after checking that the REQUEST_METHOD is POST and before branching to either resetCharacterOrder() or saveCharacterOrder(). Verify the nonce value from the POST request and use wp_verify_nonce() with the appropriate action name. Additionally, add a corresponding nonce field to the form in CharactersView that matches the nonce action and name being verified in the controller.src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php-119-134 (1)
119-134:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid sending login IPs to a hard-coded plaintext GeoIP service.
Line 131 sends each public login IP to
ip-api.comover HTTP from the login path. That leaks personal data to a third party and can be observed in transit; use a local GeoIP database, a configurable HTTPS provider with caching/privacy controls, or storeUnknownwhen no approved provider is configured.🤖 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 119 - 134, The acore_lookup_country function is sending login IPs to a hardcoded HTTP endpoint (ip-api.com), which exposes user data to a third party over an unencrypted connection. Replace this external service call with one of the following: implement a local GeoIP database lookup, use a configurable HTTPS-based provider with built-in caching and privacy controls, or return 'Unknown' as a safe default when no approved provider is configured. Remove the hardcoded wp_remote_get call to ip-api.com and implement proper privacy controls.src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php-107-116 (1)
107-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not trust client-supplied forwarding headers for security logs.
HTTP_CLIENT_IPand the firstX-Forwarded-Forvalue can be browser-supplied unless a trusted proxy strips them, so users can poison their login audit trail and current-IP highlighting. Default toREMOTE_ADDR, validate withFILTER_VALIDATE_IP, and only honor forwarded headers behind a configured trusted proxy.🛡️ Proposed hardening
function acore_resolve_client_ip() { - if (!empty($_SERVER['HTTP_CLIENT_IP'])) { - $ip = $_SERVER['HTTP_CLIENT_IP']; - } elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) { - $parts = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']); - $ip = trim($parts[0]); - } else { - $ip = $_SERVER['REMOTE_ADDR'] ?? '127.0.0.1'; - } - return sanitize_text_field($ip); + $ip = $_SERVER['REMOTE_ADDR'] ?? '127.0.0.1'; + + // If this site is deployed behind a trusted proxy, add an explicit + // trusted-proxy check before accepting X-Forwarded-For / Client-IP. + $ip = trim((string) $ip); + + return filter_var($ip, FILTER_VALIDATE_IP) ? $ip : '127.0.0.1'; }🤖 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 107 - 116, The acore_resolve_client_ip() function prioritizes client-supplied headers HTTP_CLIENT_IP and HTTP_X_FORWARDED_FOR which can be spoofed by users to poison security logs. Reorder the logic to default to REMOTE_ADDR first (which is server-supplied), only use forwarded headers if behind a configured trusted proxy, and validate the resulting IP using FILTER_VALIDATE_IP before returning to ensure the function returns only legitimate IP addresses.src/acore-wp-plugin/src/Hooks/User/User.php-491-494 (1)
491-494:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not reset expansion when the field is absent or malformed.
A profile save that does not include
acore-user-game-expansionfalls back to WOTLK and can overwrite the existing game account. Also,intval('bad')becomes0, which passes as Classic.🐛 Proposed fix
- $expansion = isset($_POST['acore-user-game-expansion']) ? intval($_POST['acore-user-game-expansion']) : null; - - if ($expansion === null || !in_array($expansion, Common::EXPANSIONS)) { + if (!isset($_POST['acore-user-game-expansion'])) { + return; + } + + $rawExpansion = wp_unslash($_POST['acore-user-game-expansion']); + $expansion = is_numeric($rawExpansion) ? (int) $rawExpansion : null; + + if ($expansion === null || !in_array($expansion, Common::EXPANSIONS, true)) { $expansion = Common::EXPANSION_WOTLK; }🤖 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 491 - 494, The current logic resets the expansion to WOTLK when the field is absent from POST data or when intval converts invalid input to 0 (which may be a valid expansion). Instead of defaulting to WOTLK when the field is missing, only update the expansion variable if the acore-user-game-expansion field is actually present in POST data and passes validation. Check if the field exists in the POST array first before attempting to validate its value, and skip setting the expansion entirely if the field is not provided, allowing the existing expansion value to be preserved rather than overwritten with a default.src/acore-wp-plugin/src/Hooks/User/User.php-632-647 (1)
632-647:⚠️ Potential issue | 🟠 MajorRefactor to use shared 2FA state helpers—deduplicate website 2FA check and align in-game logic.
The website 2FA check at lines 632–637 and 738–743 is identical to
acore_website_2fa_enabled()(ServerInfoApi.php:124–130) and should call that helper instead. The in-game check duplicates the same query pattern; extract it into a username-based variant ofacore_ingame_2fa_enabled()or refactor both locations to share the implementation. This avoids divergence between warning logic and the Security/REST API behavior.🤖 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 632 - 647, The website 2FA check logic at lines 632–637 (checking totpKey and enabledMethods) is duplicated identically elsewhere in the file and matches the existing acore_website_2fa_enabled() helper function in ServerInfoApi.php. Replace the duplicate website 2FA check logic with a call to the existing acore_website_2fa_enabled() helper, passing the user object or ID as needed. Similarly, extract the in-game 2FA check logic (the query on the account table checking totp_secret) into a username-based variant of acore_ingame_2fa_enabled() or create a shared helper function to avoid duplication. Update all locations in User.php where these checks appear to use the centralized helpers instead of repeating the implementation.src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php-328-407 (1)
328-407:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBind destructive 2FA actions to the checked account.
The Remove buttons are enabled after checking one username, but the click handlers read the current input later. Editing the field after a successful check can remove website/in-game 2FA or backup codes for a different account than the one that was verified.
🛡️ Proposed direction
function wire2fa(type, $userInput, $check, $remove, $msg, onCheck) { + var checkedUsername = ''; + + $userInput.on('input', function(){ + checkedUsername = ''; + $remove.prop('disabled', true); + }); + $check.on('click', function(){ var username = $userInput.val().trim(); if (!username) { $msg.css('color','`#d63638`').text('Enter an account name first.'); return; } @@ } else { + checkedUsername = data.username || username; $msg.css('color','`#238636`').text('2FA is active for ' + data.username + '.'); $remove.prop('disabled', false); } @@ $remove.on('click', function(){ - var username = $userInput.val().trim(); - if (!confirm('Remove ' + type + ' 2FA for ' + username + '? This cannot be undone.')) return; - $remove.prop('disabled', true).text('Removing…'); - $check.prop('disabled', true); - ajaxPost('admin/2fa-remove', { type: type, username: username }) + var username = checkedUsername; + if (!username) return; + acoreConfirm('Remove ' + type + ' 2FA for ' + username + '? This cannot be undone.', function(){ + $remove.prop('disabled', true).text('Removing…'); + $check.prop('disabled', true); + ajaxPost('admin/2fa-remove', { type: type, username: username }) .done(function(data){ $msg.css('color','`#238636`') .text('Removed on ' + data.date + ' by ' + data.staff + '. User will see a warning until they re-enable 2FA.'); @@ .always(function(){ $check.prop('disabled', false).text('Check'); if ($remove.text() === 'Removing…') $remove.text('Remove'); }); + }); }); } @@ - var username = $('`#acore-2fa-web-user`').val().trim(); + var username = $('`#acore-backup-remove`').data('checkedUsername'); if (!username) { return; }Also set
data('checkedUsername', data.username || username)only after the Website check response confirms the account whose backup-code count is being displayed.🤖 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 328 - 407, The Remove buttons become enabled after verifying a username through the check handler, but the click handlers read the current input field value at click time instead of the verified username, allowing the field to be changed between verification and removal to target a different account. In the wire2fa function, after each successful check in the .done callback when 2FA is confirmed active, store the verified username on the $remove button element using jQuery's .data() method with key 'checkedUsername' set to data.username or username. Then modify both $remove click handlers to retrieve the username from this stored data instead of reading directly from $userInput.val().trim(). Additionally, ensure the website 2FA onCheck callback stores the username the same way before displaying backup code information.src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php-98-98 (1)
98-98:⚠️ Potential issue | 🟠 MajorScope the 2FA unlock transient to the current session.
The transient key only includes the user ID, so verifying 2FA in one browser unlocks the management panel for every active session on that account until expiry. Update both the setter in the REST endpoint and the getter on the Security page to include the WordPress session token in the key.
🛡️ Keying pattern to apply consistently
- $twofaUnlocked = $websiteTotpEnabled && (bool) get_transient('acore_2fa_panel_unlock_' . $user->ID); + $unlockKey = 'acore_2fa_panel_unlock_' . $user->ID . '_' . hash('sha256', wp_get_session_token()); + $twofaUnlocked = $websiteTotpEnabled && (bool) get_transient($unlockKey);Also update the setter in
ServerInfoApi.php:470:- set_transient('acore_2fa_panel_unlock_' . $user->ID, time(), 30 * MINUTE_IN_SECONDS); + $unlockKey = 'acore_2fa_panel_unlock_' . $user->ID . '_' . hash('sha256', wp_get_session_token()); + set_transient($unlockKey, time(), 30 * MINUTE_IN_SECONDS);🤖 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` at line 98, The 2FA unlock transient key in SecurityPage.php only includes the user ID, allowing any session of that user to access the management panel once 2FA is verified in one session. Update the transient key construction in the $twofaUnlocked assignment in SecurityPage.php to include the WordPress session token (retrieved via wp_get_session_token()) along with the user ID. Additionally, update the corresponding transient setter in ServerInfoApi.php (around line 470) to use the same key format with the session token included, ensuring both the getter and setter use identical key construction for consistency.src/acore-wp-plugin/src/Hooks/User/User.php-689-713 (1)
689-713:⚠️ Potential issue | 🟠 MajorScope WP2FA hook removal to the profile.php page only.
The hook removal currently executes on every
admin_initaction except the Security subpage, butshow_user_profileandedit_user_profilehooks are only fired onprofile.phpanduser-edit.php. This causes unnecessary global hook mutations on all other admin pages. Per the comment's stated intent to remove WP2FA blocks "from the standard WordPress profile page only," guard removal with$pagenowto scope it correctly.Proposed fix
add_action('admin_init', function () { + global $pagenow; $page = isset($_GET['page']) ? $_GET['page'] : ''; - if ($page === ACORE_SLUG . '-security') return; // Security sub-page: leave hooks intact + if ($pagenow !== 'profile.php' || $page !== '') return; // Only plain Profile > Profile global $wp_filter;🤖 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 689 - 713, The anonymous function hooked to admin_init is performing unnecessary global hook mutations on every admin page instead of only on the pages where those hooks actually fire. Add a guard at the beginning of the admin_init callback to check the global $pagenow variable and return early if the current page is not profile.php or user-edit.php, so the removal of show_user_profile and edit_user_profile hook callbacks only occurs on the pages where those hooks are actually invoked.src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php-430-463 (1)
430-463:⚠️ Potential issue | 🟠 MajorEscape translated strings in JavaScript context using
esc_js(__())instead of_e().Lines 434, 438, 452, 459, and 460 use
_e()directly in single-quoted JavaScript strings. Translations containing apostrophes break the script, and malicious translations can inject arbitrary JavaScript. Useesc_js(__(...))consistently—this pattern is already applied elsewhere in the same file.Proposed fix
- msg.textContent = '<?php _e('Please enter a valid 6-digit code.', 'acore-wp-plugin'); ?>'; + msg.textContent = '<?php echo esc_js(__('Please enter a valid 6-digit code.', 'acore-wp-plugin')); ?>'; @@ - removeBtn.textContent = '<?php _e('Removing…', 'acore-wp-plugin'); ?>'; + removeBtn.textContent = '<?php echo esc_js(__('Removing…', 'acore-wp-plugin')); ?>'; @@ - msg.textContent = '<?php _e('In-game 2FA removed successfully. You can set it up again inside the game.', 'acore-wp-plugin'); ?>'; + msg.textContent = '<?php echo esc_js(__('In-game 2FA removed successfully. You can set it up again inside the game.', 'acore-wp-plugin')); ?>'; @@ - msg.textContent = (err && (err.message || (err.data && err.data.message))) || '<?php _e('An error occurred. Please try again.', 'acore-wp-plugin'); ?>'; + msg.textContent = (err && (err.message || (err.data && err.data.message))) || '<?php echo esc_js(__('An error occurred. Please try again.', 'acore-wp-plugin')); ?>'; @@ - removeBtn.textContent = '<?php _e('Remove In-game 2FA', 'acore-wp-plugin'); ?>'; + removeBtn.textContent = '<?php echo esc_js(__('Remove In-game 2FA', 'acore-wp-plugin')); ?>';🤖 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 430 - 463, Replace all instances of _e() with esc_js(__()) in the JavaScript string contexts within the fetch handlers for 2FA removal. Specifically, update the msg.textContent and removeBtn.textContent assignments that contain _e() calls—convert _e('text', 'acore-wp-plugin') to esc_js(__('text', 'acore-wp-plugin'))—on all five occurrences: the validation error message, the "Removing..." state, the success message, the error fallback message, and the button reset text. This ensures proper escaping for JavaScript context and prevents translation-based security issues.src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php-97-97 (1)
97-97:⚠️ Potential issue | 🟠 MajorSeparate TOTP-enabled check from website-2FA-enabled check.
$websiteTotpEnabledchecks onlyplugin_active && totp_enabled, but is used as a blanket gate for website 2FA status. Users with email-based 2FA alone will have$websiteTotpEnabled = false, causing the in-game 2FA block to be greyed out (line 265) with the message "Website 2FA must be set up"—even though website 2FA is actually configured via email. Instead, use a generic website-2FA-enabled flag that accounts for both email and TOTP, reserving TOTP-specific checks for verification gates that require TOTP codes (lines 216 and 306).🤖 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` at line 97, The variable $websiteTotpEnabled incorrectly gates the website 2FA status by checking only TOTP enablement, which causes email-based 2FA users to see the in-game 2FA block incorrectly greyed out. Create a separate generic website 2FA enabled flag that checks whether either email-based 2FA or TOTP is enabled (by checking if either totp_enabled or email_2fa_enabled is true in $twoFaData), and use this new flag at the availability gate for the in-game 2FA block. Preserve the TOTP-specific $websiteTotpEnabled check for actual TOTP code verification operations at lines 216 and 306 where TOTP codes are required.src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php-28-91 (1)
28-91:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRequire a fresh website-2FA verification before changing passwords.
This handler only checks the old password and nonce before updating SOAP and WordPress passwords. That does not meet the PR’s stated requirement that website 2FA protects password changes.
🤖 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 28 - 91, The PasswordHandler class is missing a website 2FA verification check before processing password changes. Add a 2FA verification requirement at the beginning of the password change validation flow, before the wp_check_password call for the old password. This verification should confirm the user has completed fresh 2FA authentication, and if not, set an error message via acore_pw_set_message, redirect to the security URL, and exit early to prevent the password change from proceeding without 2FA confirmation.src/acore-wp-plugin/src/Components/UserPanel/UserController.php-38-39 (1)
38-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the recruiter account ID before building the SOAP command.
$_POST["recruited"]can contain non-numeric text and is later interpolated intobindraf. Cast and reject invalid IDs before lookup/command construction.🛡️ Proposed fix
- $recruiterCode = $_POST["recruited"]; + $recruiterCode = absint($_POST["recruited"]); + if ($recruiterCode <= 0) { + $errorMessages[] = "Invalid recruiter value sent."; + } $existingRecruiterId = $acServices->getUserNameByUserId($recruiterCode);Also applies to: 77-77
🤖 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 38 - 39, The recruiter account ID from $_POST["recruited"] is not validated before being used in the getUserNameByUserId() call and later interpolated into the SOAP command bindraf. Add validation to cast the recruiter code to an integer and reject non-numeric values before passing it to getUserNameByUserId(). This validation should occur immediately after retrieving $_POST["recruited"] and before any further operations that use this value. Apply the same validation to the other occurrence mentioned at line 77.src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php-53-62 (1)
53-62:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCentralize password-history enforcement across all password reset paths.
This check only protects the Security page handler. The supplied profile-update and password-reset paths update
acore_password_changed_atbut do not update or enforceacore_password_history, so the “no old passwords” policy can be bypassed outside this page.🤖 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 53 - 62, The password history enforcement logic that checks acore_allow_old_passwords and validates against acore_password_history is only applied in this Security page handler, but other password reset paths bypass it. Extract the entire password history validation block (the if statement checking get_option for acore_allow_old_passwords and the foreach loop that calls wp_check_password against the password history) into a shared/reusable function or validation method, then ensure this validation is called from all password update paths (Security page, profile updates, and password resets) before allowing the password change to proceed. This ensures the no-old-passwords policy cannot be bypassed regardless of which path is used to change the password.src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php-124-130 (1)
124-130:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat email-based website 2FA as enabled.
The PR supports authenticator-app or email website 2FA, but this helper returns
falsefor email-only users. That also affects removal detection and2fa-status.🐛 Proposed fix
function acore_website_2fa_enabled(int $userId): bool { $primary = get_user_meta($userId, 'wp_2fa_enabled_methods', true); $totpKey = get_user_meta($userId, 'wp_2fa_totp_key', true); - return !empty($totpKey) && ( - (is_array($primary) && in_array('totp', $primary, true)) || - $primary === 'totp' - ); + $methods = is_array($primary) ? $primary : ($primary !== '' ? [$primary] : []); + return (in_array('totp', $methods, true) && !empty($totpKey)) + || in_array('email', $methods, true); }🤖 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 124 - 130, The acore_website_2fa_enabled function currently only checks for TOTP authenticator-app based 2FA and returns false for email-only 2FA users. Modify the function to also retrieve email-based 2FA metadata using get_user_meta (similar to how totpKey is retrieved), then update the return statement to check if either TOTP or email 2FA is enabled by including a condition that returns true when email 2FA is not empty alongside the existing TOTP checks, ensuring that users with email-based 2FA enabled are correctly identified as having 2FA enabled.src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php-458-466 (1)
458-466:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject stale TOTP keys when verifying website 2FA.
This endpoint only checks that
wp_2fa_totp_keyexists. Other code notes WP2FA may leave that key after removal, so a disabled TOTP method can still unlock the Security panel.🛡️ Proposed fix
$user = wp_get_current_user(); $rawKey = (string) get_user_meta($user->ID, 'wp_2fa_totp_key', true); - if (empty($rawKey)) + $enabledMethods = get_user_meta($user->ID, 'wp_2fa_enabled_methods', true); + $totpActive = !empty($rawKey) && ( + (is_array($enabledMethods) && in_array('totp', $enabledMethods, true)) || + $enabledMethods === 'totp' + ); + if (!$totpActive) return new \WP_Error('no_website_2fa', __('Website 2FA is not enabled on your account.'), ['status' => 400]); $data = $request->get_json_params();🤖 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 458 - 466, The current implementation only checks if the wp_2fa_totp_key metadata exists, but WP2FA may leave this key even after the TOTP method is disabled, allowing stale keys to unlock the Security panel. Add an additional validation check alongside the existing empty($rawKey) check to verify that the TOTP method is actually enabled or active (not just that the key exists), using the appropriate WP2FA metadata or status field that indicates whether the 2FA method is currently active for the user. This ensures that only users with actively enabled 2FA can proceed past the initial check.src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php-91-95 (1)
91-95:⚠️ Potential issue | 🟠 MajorDo not force a persistent auth cookie after password change.
Passing
truetowp_set_auth_cookie()creates a 14-day persistent session even if the user did not explicitly request "remember me" functionality. When a user changes their password on a security page, they should not be automatically granted a persistent session without consent.🛈 Proposed fix
wp_set_password($newPass, $user->ID); update_user_meta($user->ID, 'acore_password_changed_at', current_time('mysql')); wp_set_current_user($user->ID); - wp_set_auth_cookie($user->ID, true); + wp_set_auth_cookie($user->ID, 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/Hooks/User/PasswordHandler.php` around lines 91 - 95, The wp_set_auth_cookie() function call is passing true as the second parameter, which creates an unwanted 14-day persistent session without explicit user consent for "remember me" functionality after a password change. Modify the wp_set_auth_cookie($user->ID, true) call to pass false instead of true, or omit the second parameter entirely to default to a non-persistent session that respects the user's actual preferences during the password change flow.src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php-24-26 (1)
24-26:⚠️ Potential issue | 🟠 MajorUnslash password fields before checking or storing them.
WordPress automatically slashes request data, so passwords containing quotes or backslashes will fail the current-password verification (line 28) and be stored with incorrect characters (line 91). The password history check (line 56) will also fail for affected passwords.
🐛 Proposed fix
- $oldPass = $_POST['acore_old_pass'] ?? ''; - $newPass = $_POST['acore_new_pass'] ?? ''; - $confirmPass = $_POST['acore_confirm_pass'] ?? ''; + $oldPass = isset($_POST['acore_old_pass']) ? (string) wp_unslash($_POST['acore_old_pass']) : ''; + $newPass = isset($_POST['acore_new_pass']) ? (string) wp_unslash($_POST['acore_new_pass']) : ''; + $confirmPass = isset($_POST['acore_confirm_pass']) ? (string) wp_unslash($_POST['acore_confirm_pass']) : '';🤖 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 24 - 26, The password fields retrieved from $_POST (acore_old_pass, acore_new_pass, acore_confirm_pass) need to be unslashed before use because WordPress automatically adds slashes to request data. Apply WordPress's wp_unslash() function to each of the three password variable assignments for $oldPass, $newPass, and $confirmPass to ensure passwords containing quotes or backslashes are handled correctly during verification at line 28, password history checks at line 56, and storage at line 91.src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php-229-239 (1)
229-239:⚠️ Potential issue | 🟠 MajorAdd error handling for SOAP exceptions in the modules REST endpoint.
The
executeCommand()method can throw anExceptionif the SOAP service is not properly configured. Without a try-catch, this will cause the REST endpoint to return a fatal error instead of a proper error response. While normal SOAP failures return error strings (whichexplode()handles safely), unhandled exceptions should be caught and wrapped in aWP_Error.Proposed fix
'methods' => 'POST', 'permission_callback' => function() { return current_user_can('manage_options'); }, 'callback' => function( $request ) { - $raw = ACoreServices::I()->getServerSoap()->executeCommand('.server debug'); + try { + $raw = ACoreServices::I()->getServerSoap()->executeCommand('.server debug'); + } catch (\Throwable $e) { + return new \WP_Error('soap_error', $e->getMessage(), ['status' => 503]); + } + if (!is_string($raw)) { + return new \WP_Error('soap_error', __('Unexpected module response.', 'acore-wp-plugin'), ['status' => 503]); + } $modules = []; $capturing = 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 229 - 239, The executeCommand() method call in the module parsing logic can throw an Exception if the SOAP service is not properly configured, which will cause a fatal error instead of a proper REST endpoint error response. Wrap the entire block starting from the executeCommand() call through the foreach loop that processes the $raw output in a try-catch block, and return a WP_Error with an appropriate error message if an Exception is caught, ensuring the REST endpoint handles SOAP configuration failures gracefully.
🟡 Minor comments (8)
src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php-246-250 (1)
246-250:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHide module panels when SOAP is offline.
Line 246 shows the modules/validator panels on success, but there’s no
elsebranch to hide them on failure. After a later failed check, stale panels remain visible.Suggested fix
if (ok) { $modPanel.show(); $valPanel.show(); + } else { + $modPanel.hide(); + $valPanel.hide(); }🤖 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 246 - 250, The if statement checking the ok variable only shows the $modPanel and $valPanel panels when successful, but lacks an else branch to hide them when the SOAP check fails. Add an else block after the closing brace of the if (ok) statement that calls $modPanel.hide() and $valPanel.hide() to ensure the panels are properly hidden when the SOAP connectivity check is unsuccessful.src/acore-wp-plugin/web/assets/css/dark-mode.css-10-12 (1)
10-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the impossible nested
<body>selector.Line 10 uses
body.acore-dark-mode body.profile-php ...; abodyelement cannot be a descendant of anotherbody, so this rule never matches.Suggested fix
-body.acore-dark-mode body.profile-php `#wpbody-content` h2, -body.acore-dark-mode body.profile-php `#wpbody-content` h3 { +body.acore-dark-mode.profile-php `#wpbody-content` h2, +body.acore-dark-mode.profile-php `#wpbody-content` h3 { border-bottom-color: `#30363d`; }🤖 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 10 - 12, The CSS selector at line 10 contains an invalid nested body selector that cannot match any elements since a body element cannot be a descendant of another body element. Remove the nested body.profile-php part and instead combine both classes on a single body selector. Change the selector to target the body element with both the acore-dark-mode and profile-php classes applied simultaneously, then continue with the descendant selectors `#wpbody-content` h2 and h3 from there.src/acore-wp-plugin/web/assets/css/main.css-157-162 (1)
157-162:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
background-sizeis currently canceled by the later shorthand.Line 157 sets
background-size: cover, but Line 161 setsbackground: none, which resetsbackground-sizetoautoin the same rule.Suggested fix
.unstuck-button { - background-size: cover; border: none; color: transparent; cursor: pointer; - background: none; + background-color: transparent; + background-repeat: no-repeat; + background-position: center; + background-size: cover; padding: 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/web/assets/css/main.css` around lines 157 - 162, The background-size property set to cover is being overridden by the background shorthand property set to none later in the same CSS rule block. Remove the background: none declaration or reorder the CSS properties so that background-size: cover appears after any background shorthand properties, ensuring the background-size value is not reset. Alternatively, use a more specific property like background-color: transparent instead of the background shorthand if you only need to clear the background color while preserving the background-size setting.Source: Linters/SAST tools
src/acore-wp-plugin/web/assets/mail-return/mail-return.js-14-17 (1)
14-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset the empty-state text before each load.
After an AJAX error sets
"Failed to load mails.", the next successful empty result shows that stale error because the empty path only callsemptyWrap.show().🐛 Proposed fix
loading.show(); mailItems.empty(); + emptyMsg.text("No unread sent mails found for this character."); emptyWrap.hide(); content.hide();Also applies to: 27-30, 178-181
🤖 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 14 - 17, The empty-state text is not being reset before displaying the empty wrap, causing stale error messages like "Failed to load mails." to persist when a subsequent AJAX request successfully returns an empty result. In the sections where emptyWrap.show() is called (at lines 27-30 and 178-181 in addition to the context around line 14-17), reset the text content of emptyWrap to a default message before calling show() on it. This ensures that any previously set error messages are cleared and the correct message displays to the user.src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php-51-51 (1)
51-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the ordering value input hidden.
Line 51 renders as a visible text box by default, exposing GUIDs in the character list and allowing accidental edits.
🐛 Proposed fix
- <input name="characterorder[]" value="<?= $char["guid"] ?>"> + <input type="hidden" name="characterorder[]" value="<?= esc_attr($char["guid"]) ?>">🤖 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` at line 51, The input element with name="characterorder[]" and value="<?= $char["guid"] ?>" on line 51 in CharactersView.php is currently rendered as a visible text input box, which exposes GUIDs and allows accidental edits. Add the type="hidden" attribute to this input element to convert it to a hidden field while preserving its functionality for form submission.src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php-156-159 (1)
156-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle numeric-string
DeleteDatevalues.DB-backed Unix timestamps commonly arrive in JSON as strings, so this branch skips
* 1000and can render raw timestamps instead of formatted dates.🐛 Proposed fix
var d = item['DeleteDate']; if (d) { - var dt = new Date(typeof d === 'number' ? d * 1000 : d); - if (!isNaN(dt)) { + var dStr = String(d); + var dt = /^\d+$/.test(dStr) + ? new Date(Number(dStr) * 1000) + : new Date(dStr.replace(' ', 'T')); + if (!isNaN(dt.getTime())) {🤖 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 156 - 159, The condition checking the type of DeleteDate in the ternary operator on line 158 only handles numeric values with typeof d === 'number', but it does not account for numeric strings which are common when Unix timestamps arrive from JSON APIs. Modify the condition to also detect and properly handle numeric-string values by checking if d is a string that represents a valid number, and multiply those by 1000 just like numeric values. You can use Number(d) or parseFloat(d) to validate numeric strings, and apply the same 1000 multiplier for both actual numbers and numeric strings.src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php-78-82 (1)
78-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid showing raw SOAP errors to end users.
$soapErrorcan contain transport or host details. Show a generic message and log the raw detail server-side.🤖 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 78 - 82, The raw SOAP error details in $soapError are being exposed to end users through the acore_pw_set_message call in the PasswordHandler.php file, which could reveal sensitive information. Replace the sprintf message to show users a generic error message without including the $soapError variable, and add server-side logging to capture the actual $soapError details using the appropriate logging mechanism for debugging purposes.src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php-343-344 (1)
343-344:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRecord the admin IP in 2FA audit entries.
The response shape and PR screenshots expect removal IP details, but admin removal and backup-code removal logs never store an
ip, so later alerts can only shownull.🛡️ Proposed fix
- $log[] = ['type' => $type, 'by' => 'admin', 'timestamp' => $now, 'staff' => $staff->user_login, 'staff_id' => $staff->ID]; + $log[] = ['type' => $type, 'by' => 'admin', 'timestamp' => $now, 'staff' => $staff->user_login, 'staff_id' => $staff->ID, 'ip' => \ACore\Hooks\User\acore_resolve_client_ip()]; update_user_meta($user->ID, 'acore_2fa_admin_log', $log);- $log[] = ['type' => 'backup', 'by' => 'admin', 'timestamp' => $now, 'staff' => $staff->user_login, 'staff_id' => $staff->ID]; + $log[] = ['type' => 'backup', 'by' => 'admin', 'timestamp' => $now, 'staff' => $staff->user_login, 'staff_id' => $staff->ID, 'ip' => \ACore\Hooks\User\acore_resolve_client_ip()]; update_user_meta($user->ID, 'acore_2fa_admin_log', $log);Also applies to: 368-369
🤖 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 343 - 344, The 2FA audit log entries created in the update_user_meta calls are missing the admin's IP address field. Add the 'ip' field to the log array (which currently contains 'type', 'by', 'timestamp', 'staff', and 'staff_id') by capturing the admin's IP address and including it as a new key-value pair in the associative array before the update_user_meta call. This fix needs to be applied in multiple locations (around lines 343-344 and 368-369) where audit logs are being recorded for admin actions and backup code removals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9ea5fa3-703d-4989-b29b-5093cfafb523
📒 Files selected for processing (31)
src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.phpsrc/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.phpsrc/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.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/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
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php (2)
185-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
innerHTMLfor REST/error payload rendering.Line 187, Line 205, Line 218, and Line 224 write server/error content via
innerHTML; this is a DOM XSS sink if any response string is attacker-influenced.Proposed fix
- errorBox.innerHTML = msg; + errorBox.textContent = msg && msg.message ? msg.message : String(msg); @@ - successBox.innerHTML = data; + successBox.textContent = data; @@ - errorBox.innerHTML = data; + errorBox.textContent = typeof data === 'string' ? data : JSON.stringify(data); @@ - errorBox.innerHTML = err && err.message ? err.message : 'An error occurred.'; + errorBox.textContent = err && err.message ? err.message : 'An error occurred.';Also applies to: 203-206, 218-225
🤖 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 185 - 188, Replace all instances of innerHTML with textContent when setting error messages or server response content in the catch handlers. This prevents DOM XSS vulnerabilities by treating the content as plain text instead of HTML. Specifically, change errorBox.innerHTML = msg to errorBox.textContent = msg in the catch function block, and apply the same change to all other locations mentioned (lines around 205, 218, and 224) where server or error payloads are being rendered to the DOM.
83-84:⚠️ Potential issue | 🟡 MinorAdd
X-WP-Nonceheader to item-restore REST calls for CSRF protection.The routes at lines 24 and 32 (ToolsApi.php) only check
is_user_logged_in()and do not verify nonce. The fetch calls at lines 109 and 196 do not sendX-WP-Nonceheader. While WordPress won't reject requests without nonce in this configuration, all other authenticated REST calls in the codebase consistently include nonce for CSRF protection. ItemRestoration should follow the same pattern.Proposed fix
- var listUrl = '<?= get_rest_url(null, 'acore/v1/item-restore/list/') ?>'; - var restoreUrl = '<?= get_rest_url(null, 'acore/v1/item-restore') ?>'; + var listUrl = '<?= get_rest_url(null, 'acore/v1/item-restore/list/') ?>'; + var restoreUrl = '<?= get_rest_url(null, 'acore/v1/item-restore') ?>'; + var restNonce = '<?= esc_js(wp_create_nonce('wp_rest')) ?>'; @@ - fetch(listUrl + guid) + fetch(listUrl + guid, { + headers: { 'Accept': 'application/json', 'X-WP-Nonce': restNonce } + }) @@ - headers: { 'Accept': 'application/json', 'Content-Type': 'application/json' }, + headers: { + 'Accept': 'application/json', + 'Content-Type': 'application/json', + 'X-WP-Nonce': restNonce + },Also applies to: 109-110, 196-199
🤖 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 83 - 84, The ItemRestorationPage.php file is missing CSRF protection headers in its fetch calls. Generate a WordPress nonce using wp_create_nonce() in the PHP section and output it to JavaScript as a data attribute or global variable. Then update the fetch calls at lines 109 and 196 (the ones using listUrl and restoreUrl variables) to include the X-WP-Nonce header in their request headers object, passing the nonce value you created. This ensures consistency with other authenticated REST calls in the codebase that already implement this CSRF protection pattern.src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php (1)
166-187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack self-removal for all Website 2FA methods.
acore_2fa_sync_self_removals()only treats TOTP as enabled, so email-only Website 2FA is initialized as “not seen” and later removal is never audited. Useacore_website_2fa_enabled()here to match the feature contract.🛡️ Proposed fix
function acore_2fa_sync_self_removals(int $userId): void { - $enabled = acore_website_totp_enabled($userId); + $enabled = acore_website_2fa_enabled($userId); $seen = get_user_meta($userId, 'acore_2fa_ws_seen_enabled', true);🤖 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 166 - 187, The function acore_2fa_sync_self_removals() currently uses acore_website_totp_enabled() to check if Website 2FA is enabled, which only tracks TOTP removals and ignores email-only Website 2FA methods. Replace the call to acore_website_totp_enabled($userId) with acore_website_2fa_enabled($userId) to properly track self-removals for all Website 2FA methods as intended by the feature contract.src/acore-wp-plugin/src/Components/UserPanel/UserController.php (1)
29-41:⚠️ Potential issue | 🟠 MajorAdd nonce verification to RAF binding POST handler.
The RAF form in
RafProgressPage.php(lines 34–39) posts toUserController.php::showRafProgress()without a CSRF token. The handler at lines 29–41 reads$_POST["recruited"]and executesbindraf(line 80) without verifying a nonce. Addwp_nonce_field()to the form andwp_verify_nonce()orcheck_admin_referer()in the controller, following the pattern used inSettingsController.phpandSecurityPage.php.Also applies to: 73–80
🤖 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 29 - 41, Add CSRF nonce verification to the RAF form POST handler in the showRafProgress() method to prevent unauthorized form submissions. Add a wp_nonce_field() call to the RAF form in RafProgressPage.php, then in UserController.php at the start of the POST request handler (before processing $_POST["recruited"] around line 29), add a wp_verify_nonce() or check_admin_referer() call to validate the nonce matches the one from the form. This verification should happen before any data processing and before the bindraf execution at line 80, following the same pattern already implemented in SettingsController.php and SecurityPage.php.src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php (1)
126-143:⚠️ Potential issue | 🟠 MajorFilter out all non-public IP addresses before sending to GeoIP provider.
The current prefix-based validation is incomplete and sends non-public IPs to the external API. While using
filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)significantly improves coverage (handling 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16, 127.0.0.0/8, and IPv6 ULA/link-local ranges), it still misses the Shared Address Space range (100.64.0.0/10, per RFC 6598), which will still be leaked to ip-api.com. Implement complete public IP validation before querying the external service.🤖 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 126 - 143, The acore_lookup_country function uses incomplete prefix-based validation that fails to filter out all non-public IP addresses before sending to the external GeoIP API. Replace the current foreach loop checking against the $private_ranges array with comprehensive IP validation using filter_var with FILTER_VALIDATE_IP combined with FILTER_FLAG_NO_PRIV_RANGE and FILTER_FLAG_NO_RES_RANGE flags. Additionally, add a manual check to filter out the Shared Address Space range (100.64.0.0/10 per RFC 6598) that is not covered by the filter flags, returning "Local" if the IP falls within any of these ranges before making the remote API call.
🤖 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 58-59: The img elements for race and class icons in
MailReturnView.php are missing `alt` attributes, which prevents assistive
technologies from understanding the image content. Add `alt` attributes to both
the race-icon and class-icon img elements, using the same values already being
used for the `title` attributes (the results from AcoreCharColors::getRaceName()
and AcoreCharColors::getClassName() respectively) to ensure accessibility
compliance while maintaining consistency with the existing tooltip
functionality.
In `@src/acore-wp-plugin/src/Components/Tools/ToolsApi.php`:
- Around line 13-20: The ItemRestore method accepts the $item parameter directly
from the request and interpolates it into a SOAP command without validation,
creating a potential injection vulnerability. Before the ownership check for
$cname, add validation to ensure $item is a valid integer and reject the request
with an appropriate error if it is not. Use intval() or is_numeric() to verify
that $item contains only a valid numeric value before proceeding with the
ACoreServices SOAP command execution.
- Around line 24-35: The ItemRestorationPage.php file has two fetch calls that
are missing the required X-WP-Nonce header for REST authentication. Create a
nonce at the top of the ItemRestorationPage.php script using
wp_create_nonce('wp_rest'), then add the X-WP-Nonce header with this nonce value
to both fetch calls: the one at line 110 for the list items request and the one
at line 195 for the restore item request. Follow the implementation pattern
already used in UnstuckView.php, MailReturnView.php, and SecurityPage.php to
ensure consistency with existing code.
In `@src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php`:
- Around line 110-116: The IP extraction logic in the forwarded headers
validation section (around lines 110-116 in ConnectionLogger.php) trusts
HTTP_X_FORWARDED_FOR and HTTP_CLIENT_IP headers without validating that the
request originates from a trusted proxy. Add validation to check that the
REMOTE_ADDR server variable matches an entry in a configured trusted proxy or
CIDR list before using the forwarded headers to extract the client IP. This
prevents header spoofing attacks where an attacker can manipulate these headers
to forge arbitrary client IPs.
In `@src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php`:
- Around line 80-82: In the PasswordHandler class, the error_log call at line 81
logs sensitive information including the username (user_login) which is PII and
raw SOAP error details that may expose backend information. Replace the
user_login with the user ID (user->ID) as a non-PII identifier, and replace the
raw $soapError with a generic message that does not expose backend details. Keep
the transient message to the user generic as well without exposing raw error
information.
- Around line 24-29: The password-change 2FA verification gate in
PasswordHandler.php only checks for TOTP-enabled users via
acore_website_totp_enabled(), allowing email-based 2FA users to bypass
verification. Replace the acore_website_totp_enabled() check with the broader
acore_website_2fa_enabled() function that covers both TOTP and email-based 2FA.
Then extend the existing transient unlock mechanism (currently checked via
acore_2fa_unlock_key()) to support both TOTP and email verification paths,
ensuring all 2FA methods are validated before allowing password changes.
In `@src/acore-wp-plugin/web/assets/css/dark-mode.css`:
- Line 122: Remove the deprecated `clip` property from the hidden-radio rule in
the dark-mode.css file. The line contains `clip: rect(0 0 0 0);` which uses the
deprecated CSS property. Since the modern `clip-path` alternative is already
present on the next line, delete the `clip` property line entirely and keep only
the `clip-path` declaration.
---
Outside diff comments:
In `@src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php`:
- Around line 166-187: The function acore_2fa_sync_self_removals() currently
uses acore_website_totp_enabled() to check if Website 2FA is enabled, which only
tracks TOTP removals and ignores email-only Website 2FA methods. Replace the
call to acore_website_totp_enabled($userId) with
acore_website_2fa_enabled($userId) to properly track self-removals for all
Website 2FA methods as intended by the feature contract.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php`:
- Around line 185-188: Replace all instances of innerHTML with textContent when
setting error messages or server response content in the catch handlers. This
prevents DOM XSS vulnerabilities by treating the content as plain text instead
of HTML. Specifically, change errorBox.innerHTML = msg to errorBox.textContent =
msg in the catch function block, and apply the same change to all other
locations mentioned (lines around 205, 218, and 224) where server or error
payloads are being rendered to the DOM.
- Around line 83-84: The ItemRestorationPage.php file is missing CSRF protection
headers in its fetch calls. Generate a WordPress nonce using wp_create_nonce()
in the PHP section and output it to JavaScript as a data attribute or global
variable. Then update the fetch calls at lines 109 and 196 (the ones using
listUrl and restoreUrl variables) to include the X-WP-Nonce header in their
request headers object, passing the nonce value you created. This ensures
consistency with other authenticated REST calls in the codebase that already
implement this CSRF protection pattern.
In `@src/acore-wp-plugin/src/Components/UserPanel/UserController.php`:
- Around line 29-41: Add CSRF nonce verification to the RAF form POST handler in
the showRafProgress() method to prevent unauthorized form submissions. Add a
wp_nonce_field() call to the RAF form in RafProgressPage.php, then in
UserController.php at the start of the POST request handler (before processing
$_POST["recruited"] around line 29), add a wp_verify_nonce() or
check_admin_referer() call to validate the nonce matches the one from the form.
This verification should happen before any data processing and before the
bindraf execution at line 80, following the same pattern already implemented in
SettingsController.php and SecurityPage.php.
In `@src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php`:
- Around line 126-143: The acore_lookup_country function uses incomplete
prefix-based validation that fails to filter out all non-public IP addresses
before sending to the external GeoIP API. Replace the current foreach loop
checking against the $private_ranges array with comprehensive IP validation
using filter_var with FILTER_VALIDATE_IP combined with FILTER_FLAG_NO_PRIV_RANGE
and FILTER_FLAG_NO_RES_RANGE flags. Additionally, add a manual check to filter
out the Shared Address Space range (100.64.0.0/10 per RFC 6598) that is not
covered by the filter flags, returning "Local" if the IP falls within any of
these ranges before making the remote API call.
🪄 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: f4556f70-0dc3-4d8f-b35c-7af666632b66
📒 Files selected for processing (19)
src/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/CharactersController.phpsrc/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.phpsrc/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.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/SecurityPage.phpsrc/acore-wp-plugin/src/Components/UserPanel/UserController.phpsrc/acore-wp-plugin/src/Hooks/User/ConnectionLogger.phpsrc/acore-wp-plugin/src/Hooks/User/PasswordHandler.phpsrc/acore-wp-plugin/src/Hooks/User/User.phpsrc/acore-wp-plugin/src/Manager/ACoreServices.phpsrc/acore-wp-plugin/web/assets/css/dark-mode.csssrc/acore-wp-plugin/web/assets/css/main.csssrc/acore-wp-plugin/web/assets/mail-return/mail-return.js
🚧 Files skipped from review as they are similar to previous changes (9)
- src/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.php
- src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php
- src/acore-wp-plugin/src/Components/UnstuckMenu/UnstuckView.php
- src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php
- src/acore-wp-plugin/src/Hooks/User/User.php
- src/acore-wp-plugin/web/assets/css/main.css
- src/acore-wp-plugin/web/assets/mail-return/mail-return.js
- src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php
- src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php
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/ServerInfo/ServerInfoApi.php`:
- Around line 493-517: The request-email-2fa endpoint callback function lacks
rate limiting, allowing an authenticated user to repeatedly trigger email sends.
Before calling wp_mail to send the verification email, implement a
transient-based rate limit check that uses the current user's ID to track and
limit requests. Check if a rate limit transient exists for the user, and if it
does, return an error response indicating the user must wait before requesting
another code. Set a rate limit transient (e.g., with a duration like 1 or 2
minutes) after a successful email send to prevent abuse while allowing
legitimate retry attempts after a reasonable interval.
🪄 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: 043367cb-8dac-474d-9637-07a5a6b65762
📒 Files selected for processing (10)
src/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.phpsrc/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.phpsrc/acore-wp-plugin/src/Components/Tools/ToolsApi.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/Hooks/User/ConnectionLogger.phpsrc/acore-wp-plugin/src/Hooks/User/PasswordHandler.phpsrc/acore-wp-plugin/web/assets/css/dark-mode.css
💤 Files with no reviewable changes (1)
- src/acore-wp-plugin/web/assets/css/dark-mode.css
🚧 Files skipped from review as they are similar to previous changes (7)
- src/acore-wp-plugin/src/Components/UserPanel/Pages/RafProgressPage.php
- src/acore-wp-plugin/src/Components/Tools/ToolsApi.php
- src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php
- src/acore-wp-plugin/src/Components/UserPanel/UserController.php
- src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php
- src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php
- src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php
Contributor
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/acore-wp-plugin/src/Hooks/User/User.php (1)
105-111: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRoute all password-change paths through the same 2FA and reuse policy.
These profile/reset paths now update the Security-page timestamp, but they bypass the new
PasswordHandlerchecks for Website 2FA unlock and password reuse history. Apply the same policy before these paths can commit a password change, or centralize password changes in one shared service.Also applies to: 126-136
🤖 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 105 - 111, The password change operation in the isset($_POST['pass1']) block at lines 105-111 is calling $soap->setAccountPassword() directly, bypassing the new PasswordHandler checks for Website 2FA unlock and password reuse history. Instead of directly invoking $soap->setAccountPassword(), refactor this code path to route the password change through the PasswordHandler service class which centralizes these policy checks. This same refactoring should be applied to the other password-change path mentioned at lines 126-136 to ensure consistent security policy enforcement across all password-change operations.
♻️ Duplicate comments (2)
src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php (1)
24-29: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBlock password changes when Website 2FA is not set up.
This gate only verifies 2FA when it is already enabled; users with no Website 2FA configured can still change passwords. That contradicts the PR requirement that Website 2FA is required for password changes.
🔒 Proposed policy enforcement
- if (\ACore\Components\ServerInfo\acore_website_2fa_enabled($user->ID) - && !get_transient(\ACore\Components\ServerInfo\acore_2fa_unlock_key($user->ID))) { + if (!\ACore\Components\ServerInfo\acore_website_2fa_enabled($user->ID)) { + acore_pw_set_message($user->ID, 'error', __('Please set up Website 2FA before changing your password.', 'acore-wp-plugin')); + wp_redirect($security_url); + exit; + } + + if (!get_transient(\ACore\Components\ServerInfo\acore_2fa_unlock_key($user->ID))) { acore_pw_set_message($user->ID, 'error', __('Please verify your 2FA code before changing your password.', '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/Hooks/User/PasswordHandler.php` around lines 24 - 29, The conditional logic in the PasswordHandler.php file's password change gate currently only blocks users when 2FA is enabled but not verified. However, the requirement is that Website 2FA must be configured for any password change. Modify the if statement condition to block password changes both when 2FA is not enabled at all (add a check for when acore_website_2fa_enabled returns false) and when 2FA is enabled but not yet verified (the existing check). Combine these checks with an OR operator so that password changes are blocked in either scenario.src/acore-wp-plugin/src/Components/Tools/ToolsApi.php (1)
16-19: 🗄️ Data Integrity & Integration | 🟡 MinorReject only canonical integer item IDs.
is_numeric()accepts floating-point and scientific notation strings (e.g., "12.34", "1.2e3"), andintval()silently coerces them by truncating at the first non-integer character. This means a malformed REST payload like{"item": "12.34"}gets validated as true but coerced to ID 12, potentially targeting a different item than intended. UseFILTER_VALIDATE_INTwithmin_rangeto accept only canonical integers.🛠️ Proposed fix
- $item = $data['item']; + $item = filter_var($data['item'] ?? null, FILTER_VALIDATE_INT, ['options' => ['min_range' => 1]]); $cname = $data['cname']; - if (!is_numeric($item) || intval($item) <= 0) { + if ($item === false) { return new \WP_Error('invalid_item', 'Invalid item id.', ['status' => 400]); } - $item = intval($item);🤖 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/Tools/ToolsApi.php` around lines 16 - 19, Replace the numeric validation and type coercion in the item ID check with a stricter validation approach. Instead of using is_numeric() followed by intval(), use the FILTER_VALIDATE_INT filter with the min_range option set to 1 to strictly validate that the item parameter is a canonical integer with a minimum value greater than zero. This prevents floating-point strings like "12.34" or scientific notation from being accepted and silently coerced to incorrect integer values, ensuring only valid positive integers pass validation in the conditional block.
🧹 Nitpick comments (3)
src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
ACORE_SLUGin frontend REST URL construction.Line 84 and Line 85 hardcode
acore/v1while route registration elsewhere usesACORE_SLUG . '/v1'. Reusing the constant avoids contract drift.♻️ Proposed change
- var listUrl = '<?= get_rest_url(null, 'acore/v1/item-restore/list/') ?>'; - var restoreUrl = '<?= get_rest_url(null, 'acore/v1/item-restore') ?>'; + 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')) ?>';🤖 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 84 - 85, The REST URL construction in the listUrl and restoreUrl variables on lines 84-85 hardcodes the string 'acore/v1' instead of using the ACORE_SLUG constant. Replace the hardcoded 'acore/v1' strings with ACORE_SLUG . '/v1' in both get_rest_url() function calls to ensure consistency with route registration elsewhere in the codebase and prevent contract drift.src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php (1)
48-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden race/class icon URL rendering.
Line 48 and Line 49 compose
srcfrom DB values without strict casting + URL escaping. Cast tointand escape the final URL before output.🔧 Proposed hardening patch
-<img class="race-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getRaceName(intval($char["race"]))) ?>" src="<?= ACORE_URL_PLG . "web/assets/race/" . $char["race"] . ($char["gender"] == 0 ? "m" : "f") . ".webp"; ?>"> -<img class="class-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getClassName(intval($char["class"]))) ?>" src="<?= ACORE_URL_PLG . "web/assets/class/" . $char["class"] . ".webp"; ?>"> +<img class="race-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getRaceName(intval($char["race"]))) ?>" src="<?= esc_url(ACORE_URL_PLG . 'web/assets/race/' . intval($char['race']) . (intval($char['gender']) === 0 ? 'm' : 'f') . '.webp') ?>"> +<img class="class-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getClassName(intval($char["class"]))) ?>" src="<?= esc_url(ACORE_URL_PLG . 'web/assets/class/' . intval($char['class']) . '.webp') ?>">🤖 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 48 - 49, The race-icon and class-icon src attributes on lines 48 and 49 are constructing URLs from database values without proper type casting and output escaping. Cast the $char["race"], $char["gender"], and $char["class"] values to integers using intval() or explicit casting, then wrap the entire src URL expression with esc_url() before output to ensure the URLs are properly escaped and sanitized for HTML attribute rendering.src/acore-wp-plugin/src/Components/UnstuckMenu/UnstuckView.php (1)
55-56: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCast and escape icon path components before output.
Line 55 and Line 56 build image
srcwith raw DB values. Please cast tointand escape the final URL to avoid malformed attribute output.🔧 Proposed hardening patch
-<img class="race-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getRaceName(intval($char["race"]))) ?>" src="<?= ACORE_URL_PLG . "web/assets/race/" . $char["race"] . ($char["gender"] == 0 ? "m" : "f") . ".webp"; ?>"> -<img class="class-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getClassName(intval($char["class"]))) ?>" src="<?= ACORE_URL_PLG . "web/assets/class/" . $char["class"] . ".webp"; ?>"> +<img class="race-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getRaceName(intval($char["race"]))) ?>" src="<?= esc_url(ACORE_URL_PLG . 'web/assets/race/' . intval($char['race']) . (intval($char['gender']) === 0 ? 'm' : 'f') . '.webp') ?>"> +<img class="class-icon" height="32" width="32" title="<?= esc_attr(AcoreCharColors::getClassName(intval($char["class"]))) ?>" src="<?= esc_url(ACORE_URL_PLG . 'web/assets/class/' . intval($char['class']) . '.webp') ?>">🤖 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/UnstuckMenu/UnstuckView.php` around lines 55 - 56, The image src attributes for both the race-icon and class-icon img elements are constructed using raw database values from the $char array without proper type casting or escaping. Cast the $char["race"], $char["gender"], and $char["class"] values to integers using intval() before concatenating them into the src path string, and wrap the entire final src URL with esc_url() to properly escape the attribute output and prevent malformed HTML attributes.
🤖 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/AdminPanel/Pages/RealmSettings.php`:
- Around line 354-370: The issue is that after successfully saving module
requirements via AJAX, the referencedSlugs variable is not being updated with
the newly saved requirements. Inside the .done() handler of the AJAX call, after
the success message is displayed, extract the product slugs from the reqs array
that was just submitted and update the referencedSlugs variable to reflect the
current saved state. This ensures that subsequent validation checks use the most
recent requirements rather than the original PHP-initialized list.
- Around line 305-328: The module list rendering and "Last refreshed" timestamp
updates are currently wrapped in an if (!opts.auto) conditional in the
refreshModules function, which means automatic refreshes skip updating the
visible UI elements and leave stale module information displayed. Move the code
that updates the $list with module tags and the `#acore-modules-refreshed`
timestamp outside the if (!opts.auto) conditional block so that these UI
elements are refreshed during both automatic and manual refresh operations,
ensuring the panel always displays current module information.
In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php`:
- Line 90: The form in Tools.php lacks nonce validation, creating a CSRF
vulnerability where logged-in admins could be tricked into changing security
settings via cross-site requests. Add a WordPress nonce field to the form
element starting at line 90 using wp_nonce_field(), then validate that nonce in
the SettingsController::loadTools() method before the loop that processes and
persists the posted options. Ensure the nonce validation includes both
verification of the nonce existence and its validity using wp_verify_nonce(),
returning early if validation fails.
- Around line 526-536: The addThreshold function is interpolating the level and
days parameter values directly into HTML template strings which creates an XSS
vulnerability if tampered values are passed. Instead of interpolating these
values directly in the HTML strings for the input elements, create the input
elements without values and then use the jQuery .val() method to set the level
and days values after creation. Additionally, ensure that any server-side values
passed to addThreshold are JSON-encoded on the backend before being sent to
JavaScript to prevent injection attacks.
- Around line 122-148: When dependent form controls are disabled (such as
acore_resurrection_scroll and acore_resurrection_scroll_days_inactive), they are
excluded from the POST body submission. This causes loadTools() to not update
these values, leaving stale data persisted. Add hidden input fields that submit
explicit default values whenever the dependent controls are disabled. For the
acore_resurrection_scroll select, add a hidden input with value 0 when
!$hasResurrectionMod is true. For the acore_resurrection_scroll_days_inactive
input, add a hidden input with an appropriate default value when !$scrollEnabled
is true. Apply the same pattern to the other affected settings mentioned at
lines 178-184 and 331-337.
In `@src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php`:
- Around line 470-487: The verify-website-2fa REST endpoint callback function
currently lacks rate limiting on failed verification attempts, allowing
unlimited 6-digit code guesses. Add a transient-based failed attempt counter
using set_transient and get_transient with a user-specific key before processing
the token validation. Check the current failed attempt count at the start of the
callback and return an error if a threshold is exceeded. Increment the failed
attempt counter each time an invalid token error is returned (for the
invalid_token, wrong_token, and no_website_2fa conditions). Reset the failed
attempt counter to zero when the token validation succeeds and the unlock
transient is set. Apply this same throttling pattern to the other verification
endpoint mentioned at lines 524-541.
- Around line 289-300: The $active status check in the ServerInfoApi.php file
only verifies if TOTP 2FA is enabled but fails to account for email-based
Website 2FA protection. Modify the condition that sets the $active variable to
also check if 'email' is present in the enabledMethods metadata, using the same
logic pattern used for checking 'totp' - check if it exists in the array or if
enabledMethods equals 'email' directly. This ensures users protected by
email-based 2FA will correctly report as active in the admin status endpoint.
- Around line 190-202: The REST API route registration for 'server-info' is
missing a `permission_callback` parameter, making it publicly accessible, and
the error response exposes sensitive SOAP backend details by returning the raw
error result. Add a `permission_callback` parameter to the register_rest_route
call that checks current_user_can('manage_options') to restrict access to
administrators only, matching the pattern used in other routes in this file.
Additionally, modify the error handling in the callback function to return a
generic error message to the client instead of returning the raw $result
variable directly, and instead log the actual error details server-side for
debugging purposes.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php`:
- Around line 111-113: In all the fetch calls using the .then(function (r) {
return r.json(); }) pattern (lines 111-113, 116-118, 206-207, and 223-224), add
a check for response.ok before parsing JSON. If the response is not successful
(not a 2xx status), throw an error to prevent API error responses from being
incorrectly processed as valid data. Modify the first .then() handler to
validate r.ok is true before calling r.json(), and throw a normalized error if
the response is not successful.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php`:
- Around line 293-355: The SecurityPage.php currently restricts in-game 2FA
management to only users with TOTP-based Website 2FA enabled by checking
$websiteTotpEnabled, but the page supports email-based Website 2FA as well.
Replace both instances of the $websiteTotpEnabled condition checks (at the start
and end of the in-game 2FA section) with a more inclusive variable like
$websiteAnyEnabled or $twofaUnlocked that represents any form of Website 2FA
being active. Additionally, locate and update the remove-ingame-2fa REST
endpoint handler to accept the email unlock path alongside the existing TOTP
unlock path.
In `@src/acore-wp-plugin/src/Components/UserPanel/UserController.php`:
- Around line 220-221: The getModel() method in the UserController class returns
$this->model, but this property is never defined in the class, resulting in an
undefined property access. Define the $model property as a class member variable
(either in the constructor or as a class property declaration) and ensure it is
properly initialized with a valid model instance before the getModel() method
attempts to return it.
- Around line 16-17: The UserController constructor passes $this as an argument
to UserView on line 17, but UserView does not have a constructor defined to
accept this parameter, which will cause a runtime error. Remove the $this
argument from the UserView instantiation. Additionally, the getModel() method
references $this->model property, but this property is never declared or
initialized in the UserController class. Add a $this->model property declaration
to the UserController class (either as a class property or initialize it in the
__construct method) to ensure the property exists when getModel() is called.
In `@src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php`:
- Around line 144-162: The acore_ip_in_cidr() function casts the CIDR prefix
bits to an integer but never validates that it falls within a valid range before
using it for string indexing and bit operations. A malformed CIDR value like
/999 can cause out-of-bounds access and runtime warnings. Add a validation check
after confirming that both $ipBin and $suBin are valid and have matching lengths
to ensure that $bits does not exceed the maximum valid bit count for the IP
version (8 times the length of the binary string), and return false if the
prefix length is out of valid range.
- Around line 222-224: The wp_remote_get call in the ConnectionLogger is using
an insecure HTTP endpoint to ip-api.com instead of HTTPS, which exposes user IP
addresses and country codes in transit. Replace the
"http://ip-api.com/json/{$ip}?fields=status,countryCode" endpoint with an HTTPS
alternative (either by upgrading to the paid pro tier of ip-api.com that
supports HTTPS, switching to a different GeoIP provider that supports HTTPS, or
removing the external lookup entirely if unencrypted transport is unacceptable
for your security logging). Note that the same issue also appears in another
location around lines 296-300, so ensure you fix all instances of this HTTP
endpoint call.
In `@src/acore-wp-plugin/src/Hooks/User/User.php`:
- Around line 632-668: The code does not properly handle email-based 2FA users
and self-removal log entries in the warning blocks. First, modify the logic
determining website 2FA state to distinguish between email-based 2FA and TOTP,
so email-only users are not incorrectly treated as unprotected. Second, before
accessing the staff field in the warning display for lastWebRemoval and
lastGameRemoval entries, check if the removal was self-initiated by examining
the entry's type or structure, since self-removal entries store 'by => self' and
'ip' instead of 'staff'. Adjust the warning message display logic to handle both
staff-removal and self-removal cases appropriately, and apply the same fixes to
the similar code block referenced at lines 735-762.
In `@src/acore-wp-plugin/src/Hooks/Various/tgmplugin_activator.php`:
- Around line 42-46: The WP 2FA plugin configuration currently only includes
`required => true`, which marks the plugin as required for installation but
still allows administrators to manually deactivate it from the plugins UI. To
prevent deactivation and enforce the "cannot be disabled" requirement, add the
parameter `force_activation => true` to the WP 2FA configuration array (the one
with slug 'wp-2fa'). This will remove the deactivate button from the plugins
interface and automatically re-activate the plugin if someone attempts to
deactivate it.
In `@src/acore-wp-plugin/web/assets/css/dark-mode.css`:
- Around line 21-24: The CSS selector for `#acore-item-restoration-page .table
thead` is applying dark mode styling globally instead of only when dark mode is
active, causing the dark header background and text color to appear in light
mode as well. Prefix this selector with `body.acore-dark-mode` to scope the dark
header styling exclusively to when dark mode is enabled on the body element.
In `@src/acore-wp-plugin/web/assets/css/main.css`:
- Line 120: The keyword-case stylelint rule requires CSS color keywords to be
lowercase. In the border property at line 120, change `currentColor` to
`currentcolor` to satisfy the configured `value-keyword-case` rule and resolve
the CI lint violation.
- Around line 475-477: The mail item grid uses a fixed grid-template-columns
property with repeat(6, 56px) that does not adapt to narrow screens and causes
overflow on smaller phones. Add a media query targeting small screen sizes
(e.g., max-width breakpoints) that overrides the grid-template-columns rule with
fewer columns (such as repeat(3, 56px) or repeat(4, 56px)) to ensure the slots
wrap appropriately and prevent horizontal overflow on mobile devices.
In `@src/acore-wp-plugin/web/assets/css/theme.css`:
- Around line 230-240: The Stylelint rule `declaration-empty-line-before` is
violated in the body.wp2fa-setup selector. Add an empty line before the
background property declaration to separate the custom property declarations
(--acore-bg, --acore-surface, etc.) from the regular property declarations
(background, color). This maintains proper CSS formatting by grouping related
declarations with appropriate spacing.
---
Outside diff comments:
In `@src/acore-wp-plugin/src/Hooks/User/User.php`:
- Around line 105-111: The password change operation in the
isset($_POST['pass1']) block at lines 105-111 is calling
$soap->setAccountPassword() directly, bypassing the new PasswordHandler checks
for Website 2FA unlock and password reuse history. Instead of directly invoking
$soap->setAccountPassword(), refactor this code path to route the password
change through the PasswordHandler service class which centralizes these policy
checks. This same refactoring should be applied to the other password-change
path mentioned at lines 126-136 to ensure consistent security policy enforcement
across all password-change operations.
---
Duplicate comments:
In `@src/acore-wp-plugin/src/Components/Tools/ToolsApi.php`:
- Around line 16-19: Replace the numeric validation and type coercion in the
item ID check with a stricter validation approach. Instead of using is_numeric()
followed by intval(), use the FILTER_VALIDATE_INT filter with the min_range
option set to 1 to strictly validate that the item parameter is a canonical
integer with a minimum value greater than zero. This prevents floating-point
strings like "12.34" or scientific notation from being accepted and silently
coerced to incorrect integer values, ensuring only valid positive integers pass
validation in the conditional block.
In `@src/acore-wp-plugin/src/Hooks/User/PasswordHandler.php`:
- Around line 24-29: The conditional logic in the PasswordHandler.php file's
password change gate currently only blocks users when 2FA is enabled but not
verified. However, the requirement is that Website 2FA must be configured for
any password change. Modify the if statement condition to block password changes
both when 2FA is not enabled at all (add a check for when
acore_website_2fa_enabled returns false) and when 2FA is enabled but not yet
verified (the existing check). Combine these checks with an OR operator so that
password changes are blocked in either scenario.
---
Nitpick comments:
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php`:
- Around line 48-49: The race-icon and class-icon src attributes on lines 48 and
49 are constructing URLs from database values without proper type casting and
output escaping. Cast the $char["race"], $char["gender"], and $char["class"]
values to integers using intval() or explicit casting, then wrap the entire src
URL expression with esc_url() before output to ensure the URLs are properly
escaped and sanitized for HTML attribute rendering.
In `@src/acore-wp-plugin/src/Components/UnstuckMenu/UnstuckView.php`:
- Around line 55-56: The image src attributes for both the race-icon and
class-icon img elements are constructed using raw database values from the $char
array without proper type casting or escaping. Cast the $char["race"],
$char["gender"], and $char["class"] values to integers using intval() before
concatenating them into the src path string, and wrap the entire final src URL
with esc_url() to properly escape the attribute output and prevent malformed
HTML attributes.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php`:
- Around line 84-85: The REST URL construction in the listUrl and restoreUrl
variables on lines 84-85 hardcodes the string 'acore/v1' instead of using the
ACORE_SLUG constant. Replace the hardcoded 'acore/v1' strings with ACORE_SLUG .
'/v1' in both get_rest_url() function calls to ensure consistency with route
registration elsewhere in the codebase and prevent contract drift.
🪄 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: cb98e086-41a2-4cbe-9133-2b043bd4b930
📒 Files selected for processing (33)
src/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/CharactersController.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
+305
to
+328
| function refreshModules(opts) { | ||
| opts = opts || {}; | ||
| var $btn = opts.auto ? null : $('#acore-modules-refresh').prop('disabled', true).text('Refreshing…'); | ||
| return $.ajax({ | ||
| url: restBase + 'server-modules', method: 'POST', | ||
| beforeSend: function(xhr){ xhr.setRequestHeader('X-WP-Nonce', nonce); } | ||
| }).done(function(res){ | ||
| var newModules = (res.modules || []); | ||
|
|
||
| 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()); | ||
| } | ||
|
|
||
| renderAutoCheck(diffReferenced(knownModules, newModules)); | ||
| knownModules = newModules; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Render the refreshed module list during automatic refreshes too.
On initial connected load, refreshModules({ auto: true }) updates knownModules and the diff summary but skips the visible module tags and “Last refreshed” text, so the panel can show stale modules.
🛠️ Proposed fix
+ function renderModuleList(newModules, refreshed) {
+ 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.'));
+ }
+ if (refreshed) {
+ var d = new Date(refreshed * 1000);
+ $('`#acore-modules-refreshed`').html('Last refreshed:<br>' + d.toLocaleString());
+ }
+ }
+
function refreshModules(opts) {
@@
- 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());
- }
+ renderModuleList(newModules, res.refreshed);🤖 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 305 - 328, The module list rendering and "Last refreshed" timestamp
updates are currently wrapped in an if (!opts.auto) conditional in the
refreshModules function, which means automatic refreshes skip updating the
visible UI elements and leave stale module information displayed. Move the code
that updates the $list with module tags and the `#acore-modules-refreshed`
timestamp outside the if (!opts.auto) conditional block so that these UI
elements are refreshed during both automatic and manual refresh operations,
ensuring the panel always displays current module information.
| @@ -22,19 +89,17 @@ | |||
| <hr> | |||
| <form method="post"> | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Add nonce validation before saving the new security settings.
The new Web Integration toggles and reset action are saved through the Tools POST path, but this form does not send a nonce and loadTools() persists posted options without one. A cross-site request from a logged-in admin could change password-reuse/security-logging settings or reset name-unlock settings.
🛡️ Proposed fix
- <form method="post">
+ <form method="post">
+ <?php wp_nonce_field('acore_tools_save', 'acore_tools_nonce'); ?>Also validate it in SettingsController::loadTools() before iterating options:
if ($_SERVER['REQUEST_METHOD'] == 'POST') {
+ check_admin_referer('acore_tools_save', 'acore_tools_nonce');
+
foreach (Opts::I()->getConfs() as $key => $value) {Also applies to: 158-194, 577-585
🤖 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` at line 90,
The form in Tools.php lacks nonce validation, creating a CSRF vulnerability
where logged-in admins could be tricked into changing security settings via
cross-site requests. Add a WordPress nonce field to the form element starting at
line 90 using wp_nonce_field(), then validate that nonce in the
SettingsController::loadTools() method before the loop that processes and
persists the posted options. Ensure the nonce validation includes both
verification of the nonce existence and its validity using wp_verify_nonce(),
returning early if validation fails.
Do not enforce 2FA for password change, it's an optional use not forced. Still use the old/current system for password changing flow
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/acore-wp-plugin/src/Components/AdminPanel/SettingsController.php (1)
88-88: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftApply CSRF protection to all POST handlers.
The
loadElunaSettings()andloadPvpRewards()methods process POST requests without nonce verification, creating CSRF vulnerabilities. An attacker could trick an administrator into unknowingly submitting malicious configuration changes or triggering reward distributions.🔒 Proposed fix to add nonce checks
For
loadElunaSettings():if ($_SERVER['REQUEST_METHOD'] == 'POST') { + check_admin_referer('acore_eluna_settings_save', 'acore_eluna_settings_nonce'); + foreach (Opts::I()->getConfs() as $key => $value) {For
loadPvpRewards()(first POST check at line 135):if ($_SERVER['REQUEST_METHOD'] == 'POST') { + check_admin_referer('acore_pvp_rewards_save', 'acore_pvp_rewards_nonce'); + global $wpdb;For
loadPvpRewards()(second POST check at line 162):if ($_SERVER['REQUEST_METHOD'] == 'POST') { + // Nonce already checked at line 135 global $wpdb;Don't forget to add the corresponding
wp_nonce_field()calls in the Eluna settings and PvP rewards view templates.Also applies to: 135-144, 162-162
🤖 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/SettingsController.php` at line 88, Add CSRF protection by implementing nonce verification to all POST request handlers in the SettingsController.php file. In the loadElunaSettings() method (POST check at line 88), add a check for wp_verify_nonce() with an appropriate nonce action before processing the POST data. Similarly, in the loadPvpRewards() method, add nonce verification checks at both POST handler locations (lines 135-144 and 162). Additionally, add corresponding wp_nonce_field() calls in the view templates for Eluna settings and PvP rewards forms to generate the nonce fields that will be sent with the POST requests.
🤖 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.
Outside diff comments:
In `@src/acore-wp-plugin/src/Components/AdminPanel/SettingsController.php`:
- Line 88: Add CSRF protection by implementing nonce verification to all POST
request handlers in the SettingsController.php file. In the loadElunaSettings()
method (POST check at line 88), add a check for wp_verify_nonce() with an
appropriate nonce action before processing the POST data. Similarly, in the
loadPvpRewards() method, add nonce verification checks at both POST handler
locations (lines 135-144 and 162). Additionally, add corresponding
wp_nonce_field() calls in the view templates for Eluna settings and PvP rewards
forms to generate the nonce fields that will be sent with the POST requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aab296c-7232-4b05-b5f6-2bb69fbb7e37
📒 Files selected for processing (16)
src/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/CharactersView.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/SecurityPage.phpsrc/acore-wp-plugin/src/Components/UserPanel/UserController.phpsrc/acore-wp-plugin/src/Hooks/User/ConnectionLogger.phpsrc/acore-wp-plugin/src/Hooks/User/User.phpsrc/acore-wp-plugin/src/Hooks/Various/tgmplugin_activator.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.css
🚧 Files skipped from review as they are similar to previous changes (15)
- src/acore-wp-plugin/src/Hooks/Various/tgmplugin_activator.php
- src/acore-wp-plugin/src/Components/Tools/ToolsApi.php
- src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php
- src/acore-wp-plugin/src/Components/AdminPanel/Pages/RealmSettings.php
- src/acore-wp-plugin/src/Components/UnstuckMenu/UnstuckView.php
- src/acore-wp-plugin/src/Components/UserPanel/UserController.php
- src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php
- src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php
- src/acore-wp-plugin/web/assets/css/dark-mode.css
- src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php
- src/acore-wp-plugin/web/assets/css/main.css
- src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php
- src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php
- src/acore-wp-plugin/src/Hooks/User/User.php
- src/acore-wp-plugin/web/assets/css/theme.css
added missing nonce/csrf
added missing nonce/csrf and save states.
This was referenced Jun 23, 2026
Merged
Contributor
Author
|
Closing this PR as it has been splited already (check the mentiones ab ovE) |
Nyeriah
pushed a commit
that referenced
this pull request
Jul 5, 2026
Split from the main PR: #197 Split by Claude Unstuck restyled to the new rows; Scroll of Resurrection gets green/red/yellow status colours; RAF (recruit-a-friend) gets a CSRF nonce + validation. Minor shared-service tidy. <img width="1920" height="1030" alt="image" src="https://github.com/user-attachments/assets/97b69047-870e-41bd-878d-5c52a665384a" /> <img width="1920" height="1030" alt="image" src="https://github.com/user-attachments/assets/dca4c6e3-b29d-4175-a227-84e7d52a730a" /> Not testde the RAF page, as the changes were made by Rabbit's review. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Dark mode toggle in WordPress admin bar * Character order reset functionality in character management * Item quality indicators in mail and restoration displays * **Improvements** * Redesigned character management UI with card-based layout, icons, and metadata * Revamped mail return interface with character sidebar navigation * Reimagined item restoration page with card grid and improved organization * Enhanced security with nonce validation and permission checks across endpoints * Improved visual styling with expanded theme and dark mode CSS * **Bug Fixes** * Module availability validation for Resurrection Scroll feature <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Yehonal
pushed a commit
to azerothcore/acore-cms-wp-plugin
that referenced
this pull request
Jul 5, 2026
Split from the main PR: azerothcore/acore-cms#197 Split by Claude Unstuck restyled to the new rows; Scroll of Resurrection gets green/red/yellow status colours; RAF (recruit-a-friend) gets a CSRF nonce + validation. Minor shared-service tidy. <img width="1920" height="1030" alt="image" src="https://github.com/user-attachments/assets/97b69047-870e-41bd-878d-5c52a665384a" /> <img width="1920" height="1030" alt="image" src="https://github.com/user-attachments/assets/dca4c6e3-b29d-4175-a227-84e7d52a730a" /> Not testde the RAF page, as the changes were made by Rabbit's review. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Dark mode toggle in WordPress admin bar * Character order reset functionality in character management * Item quality indicators in mail and restoration displays * **Improvements** * Redesigned character management UI with card-based layout, icons, and metadata * Revamped mail return interface with character sidebar navigation * Reimagined item restoration page with card grid and improved organization * Enhanced security with nonce validation and permission checks across endpoints * Improved visual styling with expanded theme and dark mode CSS * **Bug Fixes** * Module availability validation for Resurrection Scroll feature <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Nyeriah
pushed a commit
that referenced
this pull request
Jul 5, 2026
Split from the main PR: #197 Split by Claude Full overhaul: confirm prompt before returning, a 6×2 grid matching the 12-item mail cap, shows the recipient's faction/class, Wowhead tooltips + higher-res icons, and clean empty/loaded states. <img width="1920" height="1030" alt="image" src="https://github.com/user-attachments/assets/e3ed47b2-582e-40ec-a218-6ddc998be498" /> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added dark mode toggle for WordPress admin dashboard * **Improvements** * Redesigned character ordering screen with visual enhancements and reset order option * Redesigned mail return interface with sidebar character navigation * Enhanced mail display with item quality information and improved metadata * Expanded admin styling and dark mode support across all admin pages <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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 quite of a big PR and many aspects of were pointed, cleaned up or suggested (code wise) by AI (Claude anthropic) not exclusively AI but helped by AI.
The only thing of this PR that I have not tested it's the IP logging for IPv4, mock data worked and Locally (using local IPs) worked, but did not test public IPs, code and logically wise it should work.
Dark Mode
The users now have the option to choose between dark mode and keep it light (still the default) and it will remember on the user's settings (part of the wordpress DB), this setting supports all user and adminstrator (AC) related pages.
White
Dark
Security Tab
and
Have been both moved from Profile to the new Security Tab, what does the Security Tab bring and do now?
Will now tell you (from now and onwards) the last time you password was changed
Also depending on your adminstrator settings, you may or not be able to re-use your old password.
Under the AzerothCore > Tools > Web Integration
Security logging, if enabled will log the IP connections on the website and the last in-game one if it exists, if disabled doesn't do anything.
And for "Allowing Old Password" just allows you re-use an old passwords (currently it only saves up to 10, this is hashed not plained text, so same behaviour and same check the website and core do via the database to see if it's valid or not password before storing).
Back to the security tab, now part of it you also ahe the dedicated 2FA Fields for Website and In-game!
Supports both Authecator app or E-mail.
After having a 2FA actively on the account, it will be required for the login and any Password changes:
In the removal of the 2FA for either the in-game or website you will be informated! (in security tab and top of profile page)
This message will display for either user removal or staff's removal, so no "hidden" removal of security features of an user, but still gives the admisntrator a way to remove them in the case the user can prove their are the owner but they can't access it.
In-game setup is done fully in-game, but to be able to manage it on the website (to remove the 2FA in-game) it requires you to have 2FA setup on the website first as security measure.
(cannot change it until i get myself 2FA in the websit)
In the admin side of things you check and remove either of the 2FA
And the last point of the secuirty tab is full list of login IPs (from most recent to oldest) in Security tab shows up to 50 (can expand infinite every 50 entries), yellow selection = matches your current IP (in-game or website).
(top/default)
(expanded)
(profile bottom page, shows up to 10 latest, you can click see more to take to security tab)
--
Character selectors got a rework!
Character Selector Row Meaning? Idk how to phrase.
-- Icons and badges support tooltips
-- Left Tab = Class Colour
-- Border (3 sides and selection) = Faction Colour
-- Levels have a badge and colour (brown/vanilla = 0-60, green/tbc = 61-70 and blue/wrath = 71-80+)
-- Race (uses Faction colour Border) and slightly bigger
-- Class (uses class colour border) and slightly bigger
-- Heartstone just has a border (looks prettier)
Also now has a reset order (sets to NULL on the DB) for the times order soft-locks a characters from being logged into)
Dark
White
Re-ordered
Complety overhaul! More intuative to users i owuld say.
If no mails found
If mails found (dark theme)
Re uses wowhead tooltips and higher quality icons
(Mails found but white theme)
Just got visuals updated to easier to tell the user if it's enrolled or not by having green, yellow or red colours (green active, red not active, yellow informative)
If no items found
If items found (white theme)
(dark theme)
To finish the profile tab!
Warning if 2FA was removed on the top of the page
IP logs on the bottom (latest 10 connections)
and a better UI for users to select their Expansion and a note.
Admin Stuff
Added a required plugin: 2FA (from wp-2fa) (cant still be disabled)
Realm Settings
Had a few changes to it, still has and uses the manual "Check for SOAP" button, but upon saving, if connection exists, it will staight up tell you.
SOAP Settings Badge
The server needs to be fully running to accept SOAP check otherwise is considered not reacahble/offline/wrong info
(world server running and soap connection working)
(world server not running or not working)
You're now able to get current server module lists (for their module string be more pricesely)
Which is used to validate or not dependcies need (sadly the "Module Requirements" has to be define in php, probably a better altnerative could be in the future) to define a page "depended" of a module.
And in this case the module is missing (in my case not active)
So it also doesnt appear on th euser side
Scroll of Resurrection section associated with the module mod-resurrection-scroll
Security Logging, track or not track IPs for the users Login History
Allow Old password, allow or not the old passwords
Manual check 2FA and if the user has Generated Backup codes, and remove the 2FA if needed.
You also can search for an account's IP history
(top, default)
(bottom)
(bottom +50)
Summary by CodeRabbit