[6 / 7] - Security tab #203
Open
TheSCREWEDSoftware wants to merge 6 commits into
Open
Conversation
…nection logging + GeoIP
📝 WalkthroughWalkthroughThis PR adds a Security user panel page (password change with history enforcement and SOAP sync, website/in-game 2FA gates using TOTP and email, admin removal logging, recent connections with GeoIP); a new ConnectionLogger module logging login history with ip-api GeoIP backfill; a Dark Mode admin bar toggle; a shared ChangesSecurity, 2FA, Connection Logging, Dark Mode & UI Overhaul
Sequence Diagram(s)sequenceDiagram
participant User
participant SecurityPage
participant REST as WP REST API
participant ServerInfoApi
participant GameDB as Game DB
participant WP2FA as WP2FA Plugin
rect rgba(100, 149, 237, 0.5)
Note over User,WP2FA: Website 2FA TOTP Gate
User->>SecurityPage: enter 6-digit TOTP code
SecurityPage->>REST: POST verify-website-2fa
REST->>ServerInfoApi: acore_wp2fa_code_is_valid()
ServerInfoApi->>WP2FA: is_valid_authcode (preferred)
WP2FA-->>ServerInfoApi: valid
ServerInfoApi->>ServerInfoApi: set unlock transient
ServerInfoApi-->>SecurityPage: 200 OK
SecurityPage->>SecurityPage: reveal WP2FA management panel
end
rect rgba(255, 140, 0, 0.5)
Note over User,GameDB: In-game 2FA removal
User->>SecurityPage: submit optional TOTP + remove
SecurityPage->>REST: POST remove-ingame-2fa
REST->>ServerInfoApi: check unlock transient or fresh TOTP
ServerInfoApi->>GameDB: UPDATE account SET totp_secret=NULL
GameDB-->>ServerInfoApi: ok
ServerInfoApi-->>SecurityPage: success
SecurityPage->>SecurityPage: reload page after delay
end
rect rgba(34, 139, 34, 0.5)
Note over User,SecurityPage: Password change
User->>SecurityPage: submit new password
SecurityPage->>REST: admin_init PasswordHandler
REST->>REST: validate old/new, check history
REST->>GameDB: SOAP setAccountPassword
GameDB-->>REST: ok
REST->>REST: wp_set_password + record history
REST-->>SecurityPage: redirect with success
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (2)
src/acore-wp-plugin/web/assets/mail-return/mail-return.js (1)
43-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
expansionColor()appears unused.This helper is defined within the success callback, but the level badge (Lines 109-114) derives its
data-expslug inline vialvlExpand relies on CSS for coloring.expansionColoris not referenced within the callback scope. Consider removing it as dead code.🤖 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 43 - 47, The `expansionColor()` function defined within the success callback is not referenced anywhere in the callback scope and represents dead code. The level badge styling (lines 109-114) derives its color through CSS classes based on the `data-exp` attribute rather than using the `expansionColor()` function. Remove the entire `expansionColor()` function definition as it is unused and unnecessary.src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php (1)
399-402: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent confirmation UX in
wire2faremoval.This handler uses the native
confirm()dialog, while the rest of the page (backup-code removal, threshold delete, reset) routes through the styledacoreConfirmmodal defined at Line 551. Consider usingacoreConfirmhere too for consistent look/behavior (note its callback is async, so the removal logic moves inside the callback).🤖 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 399 - 402, Replace the native confirm() dialog in the wire2fa removal handler with the acoreConfirm modal for consistent styling and behavior across the page. Remove the synchronous confirm() check and instead wrap the removal logic (username assignment and subsequent operations) inside the async callback of acoreConfirm, passing the confirmation message and callback function to maintain the same user experience as other removal handlers like backup-code removal and threshold delete already implemented on this page.
🤖 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/Tools.php`:
- Around line 250-251: The input element with the name attribute
acore_name_unlock_allowed_banned_names_table is directly echoing the value from
Opts::I()->acore_name_unlock_allowed_banned_names_table into the HTML value
attribute without escaping, which creates a stored XSS vulnerability. Wrap the
echoed value with the esc_attr() function to properly escape it before rendering
it into the attribute, matching the approach already applied to the numeric
input at Line 149. This ensures that any special characters like quotes or
script tags are safely encoded and cannot break out of the attribute.
- Around line 595-601: Add a guard clause before the foreach loop that iterates
over Opts::I()->acore_name_unlock_thresholds to check if the value is actually
an array using is_array(). If the value is not an array, assign an empty array
to a local variable instead. Then iterate over this validated variable in the
foreach loop instead of directly using Opts::I()->acore_name_unlock_thresholds.
This prevents warnings in PHP 7.x and TypeErrors in PHP 8 when the database
value has been corrupted or overwritten with a non-array value.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.php`:
- Around line 49-56: In the resetCharacterOrder method, the account ID retrieved
from getAcoreAccountId() is not validated before being used in the bindValue
call on line 55. If getAcoreAccountId returns a non-numeric or false value, the
update query could execute with incorrect account scope. Add validation
immediately after retrieving $accId to ensure it is a positive integer, and only
execute the prepare, bindValue, and executeQuery statements if the validation
passes.
In `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.php`:
- Around line 72-85: The current character reordering implementation using
jQuery sortable on line 74 only supports drag-and-drop interaction, which is not
accessible to keyboard-only users. Add an accessible alternative reordering
mechanism by implementing per-row up/down button controls (e.g., move up/move
down buttons for each character row) that allow keyboard navigation and
interaction. These controls should manipulate the same characterorder[] input
field values and update the position display (acore-char-pos) just like the
sortable implementation does, ensuring the same data structure is maintained
when the order is submitted.
- Around line 48-49: The race-icon and class-icon img tags in the
CharactersView.php file are missing alt attributes, which are required for
accessibility and assistive technologies. Add alt attributes to both img
elements (the one with class="race-icon" and the one with class="class-icon")
using the same escaped values from their respective title attributes, which call
AcoreCharColors::getRaceName() and AcoreCharColors::getClassName() methods.
In `@src/acore-wp-plugin/src/Components/ServerInfo/ServerInfoApi.php`:
- Around line 152-159: The function acore_ingame_2fa_enabled accepts a $userId
parameter but ignores it and instead calls $services->getAcoreAccountId() to
retrieve the current session account ID. This causes the function to always
check the 2FA status for the current user rather than the requested user.
Replace the call to getAcoreAccountId() with the $userId parameter that was
passed to the function so that the query checks the correct user's 2FA status.
- Around line 447-456: The remove-ingame-2fa endpoint does not rate-limit TOTP
code validation attempts, making it vulnerable to brute force attacks despite
requiring authentication. After each failed TOTP validation in this endpoint
(whether from format validation with preg_match or code validation with
acore_wp2fa_code_is_valid), increment and check a rate-limiting transient
similar to what verify-website-2fa uses for its 5-attempt limit. Before
processing any fresh TOTP code validation, check if the rate limit transient has
been exceeded and return an error if so. Use a consistent transient key
mechanism based on acore_2fa_unlock_key or similar to track failed attempts
across this endpoint.
- Around line 200-207: The code calls stripos($result, $pattern) on the result
from ServerInfoApi::serverInfo() without validating that $result is a string,
which will cause a TypeError if the method throws an exception or returns a
non-string value. Wrap the ServerInfoApi::serverInfo() call in a try-catch block
to handle any exceptions, then add type validation to ensure $result is a string
before iterating through $errorPatterns and calling stripos(). Follow the
exception handling and response type validation pattern used in the
server-modules endpoint (referenced in the comment as lines 247-254) to ensure
consistent error handling throughout the code.
In `@src/acore-wp-plugin/src/Components/Tools/ToolsApi.php`:
- Around line 30-31: The callback function for the REST endpoint needs to
implement error handling to convert expected exceptions from character lookup
failures into REST errors. Wrap the call to ToolsApi::ItemRestoreList within a
try-catch block, catch exceptions that may be thrown by
getRestorableItemsByCharacter() (such as those for invalid or non-owned
characters), and return a WP_Error object with an appropriate error code and
message instead of allowing the exception to propagate as a generic server
failure.
- Around line 19-22: The executeCommand method call is missing authorization for
the recovery item itself. While the code verifies that the character belongs to
the current account using ACoreServices::I()->currentAccountOwnsCharacterName(),
it does not validate that the recovery_item (identified by the item variable)
belongs to that character and account. Add a helper method that performs a
joined lookup validating that the recovery_item.Id matches the provided
character's guid, belongs to the current account, and is not deleted (checking
deleteDate IS NULL). Execute this authorization check before calling
executeCommand to prevent unauthorized item restoration.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php`:
- Around line 199-202: The error message is being set in the errorBox within the
catch block, but the errorBox is inside the hidden `#item-restore-content`
container that was hidden earlier at line 117. Before setting the error message
in the errorBox, unhide the `#item-restore-content` container by setting its
display property to visible or block. This ensures the error message becomes
visible to the user when the catch block executes and populates the errorBox
with the error message.
- Around line 217-235: Replace the fragile string matching logic in the restore
response handler with structured response checking. Change the condition that
currently checks if data is a string containing 'mail' to instead check if
data.success equals true. When success is true, extract the message from
data.message instead of using the raw data string for the successBox.textContent
assignment. Keep the remaining card removal, renumbering, and content visibility
logic unchanged. This ensures the success detection is based on an explicit flag
rather than brittle text matching.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/RafProgressPage.php`:
- Around line 14-21: The conditional on line 14 in RafProgressPage.php is
checking the wrong configuration key for gating the same-IP warning notice.
Currently it checks end_raf_on_same_ip, but the backend enforcement in
UserController::showRafProgress() uses check_ip instead. Update the condition on
line 14 to check Opts::I()->eluna_raf_config['check_ip'] === '1' instead of
end_raf_on_same_ip to ensure the frontend warning text aligns with the actual
server-side enforcement behavior.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/SecurityPage.php`:
- Around line 220-267: The WP2FA management HTML stored in the $wp2faHtml
variable is being rendered inside hidden panels (display:none) in both the
$websiteTotpEnabled and $websiteEmailEnabled branches, which exposes sensitive
backup codes and settings in the page source before verification. Remove the
$wp2faHtml from the hidden acore-2fa-panel divs in both the TOTP gate and email
gate branches so the management UI is never present in the DOM before the gate
passes. After verification succeeds and the unlock transient is set, reload the
page or fetch the protected panel server-side to ensure acore-2fa-panel is only
rendered with its HTML content when the user has been properly authenticated.
In `@src/acore-wp-plugin/src/Components/UserPanel/UserController.php`:
- Around line 54-68: The IP checking logic in UserController.php is vulnerable
to spoofing because it directly accepts HTTP_CLIENT_IP and HTTP_X_FORWARDED_FOR
headers without validation. Replace the manual IP extraction code that checks
$_SERVER['HTTP_CLIENT_IP'], $_SERVER['HTTP_X_FORWARDED_FOR'], and
$_SERVER['REMOTE_ADDR'] with a call to the trusted helper function
acore_resolve_client_ip() which already validates against configured trusted
proxies. Use the return value from this helper function to set activeUserIp
instead of the current approach that applies the wpb_get_ip filter manually.
- Around line 217-221: The getModel() method in UserController is dead code that
is never called and contains a misleading docblock stating it returns a
non-existent UserModel class while actually returning $this. Remove the entire
getModel() method definition including its docblock from the UserController
class since it has no dependencies and serves no purpose.
In `@src/acore-wp-plugin/src/Hooks/User/ConnectionLogger.php`:
- Around line 34-58: The acore_log_login function makes blocking HTTP requests
during the login flow via acore_lookup_country calls, which can stall the login
by up to 6 seconds. Replace the acore_lookup_country($ip) call with
acore_geoip_known_country($ip) to use only the local database cache instead of
making synchronous HTTP requests. Store Unknown as the country value when the IP
is not found in the local cache, and allow the existing 5-minute backfill cron
job to resolve these Unknown rows asynchronously using the batch endpoint,
completely removing blocking outbound calls from the login path.
In `@src/acore-wp-plugin/src/Hooks/User/User.php`:
- Around line 759-764: The notice displays $lastWebRemoval['staff'] without
checking if it exists, causing an undefined-index warning when the entry is a
self-removal (where staff is not set). Modify the printf statement to
conditionally display different text based on whether $lastWebRemoval['staff']
is set. If the staff key exists, display the current message with the staff
member name. If it does not exist (self-removal case), display an alternative
message indicating that the user removed their own Website 2FA. Use isset() or
array_key_exists() to safely check for the staff key before accessing it.
In `@src/acore-wp-plugin/web/assets/mail-return/mail-return.js`:
- Around line 98-104: The safeName variable is not properly escaped for HTML
attribute context, specifically for double quotes. When safeName is interpolated
into the title and data-wowhead attributes, double quote characters in item
names will break out of the quoted attribute value, causing malformed markup.
Replace all instances where safeName is used within the attribute values (inside
title="..." and data-wowhead="..." on the lines building itemsHtml) with a
version that escapes double quotes as HTML entities. Use a method that replaces
" with " when safeName is placed into these attribute contexts to prevent
attribute injection.
---
Nitpick comments:
In `@src/acore-wp-plugin/src/Components/AdminPanel/Pages/Tools.php`:
- Around line 399-402: Replace the native confirm() dialog in the wire2fa
removal handler with the acoreConfirm modal for consistent styling and behavior
across the page. Remove the synchronous confirm() check and instead wrap the
removal logic (username assignment and subsequent operations) inside the async
callback of acoreConfirm, passing the confirmation message and callback function
to maintain the same user experience as other removal handlers like backup-code
removal and threshold delete already implemented on this page.
In `@src/acore-wp-plugin/web/assets/mail-return/mail-return.js`:
- Around line 43-47: The `expansionColor()` function defined within the success
callback is not referenced anywhere in the callback scope and represents dead
code. The level badge styling (lines 109-114) derives its color through CSS
classes based on the `data-exp` attribute rather than using the
`expansionColor()` function. Remove the entire `expansionColor()` function
definition as it is unused and unnecessary.
🪄 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: 3cb53f54-e9eb-4eb1-97fa-a4ad1f265af3
📒 Files selected for processing (31)
src/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/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
TheSCREWEDSoftware
added a commit
to TheSCREWEDSoftware/acore-cms
that referenced
this pull request
Jun 24, 2026
(Resume generated by Claude) **PR azerothcore#199 — Characters Menu** - Fixed typo: "succesfully" → "successfully" in save confirmation - Fixed broken sentence in the Order Character Screen description - Extended `user-select: none` to cover all `.acore-char-row` elements, not just `button` **PR azerothcore#200 — Mail Return** - *(issues covered by later PRs — no unique fixes)* **PR azerothcore#201 — Item Restoration** - Added `alt=` attributes to race/class icons in `CharactersView.php` - `ToolsApi`: `ItemRestoreList` now catches exceptions and returns a 403 instead of a 500 - `ToolsApi` + `ACoreServices`: **Security fix** — `ItemRestore` now verifies the recovery item belongs to the character, not just that the user owns the character name (IDOR vulnerability) **PR azerothcore#202 — Unstuck / Scroll / RAF** - `ItemRestorationPage`: removed brittle `data.includes('mail')` success check — any HTTP success now triggers the success path **PR azerothcore#203 — Security Tab** - `CharactersController`: `resetCharacterOrder()` now guards against invalid/null account ID before running UPDATE - `Tools.php`: added `esc_attr()` on banned-names table input (XSS fix) - `Tools.php`: added `is_array()` guard on thresholds `foreach` - `Tools.php` + `SettingsController`: added sentinel input so deleting all thresholds actually clears them in the DB - `Tools.php`: GeoIP hidden fallback `value="0"` is now always rendered, not conditionally **PR azerothcore#204 — Admin Settings / CSRF** - `AcoreCharColors`: removed unused `$fDark`, unknown race IDs now get a neutral color instead of Horde red - `MailReturnView`: added `esc_url()` on race/class icon `src` attributes - `DarkMode`: added `dmToggleInFlight` flag to prevent overlapping AJAX requests on rapid dark mode clicks
Contributor
Author
|
The reviews are valid for this, but the are fixed in the on the way to latest PR and including he last PR #205 PR #203 — Security Tab
|
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.
Split from the main PR: #197
Split by Claude
New Security tab with password change (shows last-changed date, optional no-reuse of last 10 hashed passwords) and 2FA for Website (authenticator or email) and In-game. 2FA becomes required for login + password changes; removals notify the user (security tab + top of profile, whether self or staff). Admin tools to check/remove a user's 2FA + backup codes. Connection logging (website + last in-game IP, opt-in) with GeoIP country lookup, plus the Recent-Connections lists (10 on profile, 50-at-a-time on the security tab, current IP highlighted). Makes WP-2FA a required, non-disableable plugin.
Mentioned here #201 fixed in current PR
Summary by CodeRabbit
Release Notes
New Features
Improvements
Security