fix(13-06,13-13): session invalidation, email-service crash, and recovery-command reachability - #18
Conversation
- EmailVerificationService.getEmailService() now resolves EmailService via plugin.getContext().getBean(EmailService.class) instead of the framework-internal ContextHolder, which the framework deleted outright (framework #390, 2026-09-04) -- the module no longer compiles at all without this change. - EmailVerificationServiceTest no longer installs a stub into that holder or injects the emailService field via reflection; it stubs the mock plugin's getContext() instead, so the test now exercises the real lazy-init lookup path production uses. - Executed ahead of Task 1 in this plan's own numbering: the ContextHolder import broke the module's main compile phase, which blocks every test in the module regardless of -Dtest filter, including Task 1's own LoginServiceTest verification. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
- New Session Invalidation nested group in LoginServiceTest: one site-level test per
call site that must invalidate a player's session (unregister, resetPassword(UUID),
resetPassword(UUID, String), changePassword), plus an end-to-end deletion test
(unregister -> hasValidSession false -> rejoining does not auto-login) and an
end-to-end recovery test driven through EmailVerificationService.resetPasswordAfterRecovery
to prove the delegation reaches the real invalidation.
- Also a negative-path test proving a rejected password change does not invalidate the
session, matching the plan's success-branch-only contract.
- Updated the existing "force logout an online player" unregister test: it used to assert
player.sendMessage(...) fired as an observable proxy for "onPlayerJoin replayed"; now it
asserts that replay never happens (never().sendMessage / never().getLocation()), since
removing the replay is the fix this plan makes, not an implementation detail.
RED, captured before any production change (LoginService.java untouched):
Tests run: 131, Failures: 6, Errors: 1
- 6 new SessionInvalidation tests fail on "Expecting value to be false but was true" --
the session survives every credential-affecting call because invalidateSession does not
exist yet.
- The updated Unregister test errors with a NullPointerException inside the still-present
onPlayerJoin() replay (Player.getLocation() returns null on this mock), which is itself
further evidence the replay this plan removes is still live.
Full transcript: /home/wisdomme/servers/evidence/phase-13/13-LEDGER-UltiLogin.md
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
…anges
- New LoginService.invalidateSession(UUID): removes every session map entry whose key
ends with ":<playerUuid>", so a session opened from a different address than the
caller's own is also ended. No second identifier-to-keys index -- the session map
already holds one entry per authenticated address per player, so a scan is bounded.
- Wired from all four credential-affecting paths, success branch only:
- unregister(UUID): invalidates before touching the online player, and no longer
replays onPlayerJoin() to force a logout -- that replay was the defect (it re-ran
the session check, which found the never-cleared session and logged the deleted
account straight back in).
- resetPassword(UUID) (administrator reset)
- resetPassword(UUID, String) (specific password -- also what
EmailVerificationService.resetPasswordAfterRecovery delegates to, so the recovery
path inherits the invalidation without a second call site)
- changePassword(UUID, String, String)
- All 131 LoginServiceTest tests green, including the 6 new session-invalidation tests
and the updated forced-logout regression test.
Full RED/GREEN transcripts per call site: /home/wisdomme/servers/evidence/phase-13/13-LEDGER-UltiLogin.md
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
Dedicated unit test proving invalidateSession(UUID) removes a session opened from a different address than the one the caller happens to be connected from -- the exact behavior an administrator ending someone else's session, or a recovery flow, needs. Directly exercises the suffix-match key removal rather than going through a specific call site, complementing the four site-level tests already in this nested group. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
- anUnauthenticatedPlayerCanReachTheRecoveryCommandOnAnUpgradedServer: RED, fails against unfixed UltiLogin.reloadSelf() (expected true, got false) -- a corrected login.yml plus a real reload still leaves /recover unreachable - theListTheRunningPluginHoldsMatchesWhatWasMeasured: RED, the in-memory allowedCommands list still lacks regs/recover after the reload - anUnauthenticatedPlayerStillCannotReachACommandThatIsNotPermitted: green from the start (negative control, matches 13-06's failedChangePasswordDoesNotInvalidateSession pattern) Drives the REAL ConfigManager, LoginConfig.init()/reloadConfigs binding, and UltiLogin.reloadSelf() method body (not a re-implementation or a stub of any of the three), against a stored configuration in the shape an upgraded server actually has, per 13-LEDGER-UltiLogin.md's "Recovery command diagnosis". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
UltiLogin#13: /ul reload UltiLogin logged "UltiLogin 配置已重载!" without ever calling ConfigManager.reloadConfigs(this) -- the only thing that re-reads login.yml into a running LoginConfig. On an upgraded server, allowedCommands (and every other @ConfigEntry field on LoginConfig) was frozen at whatever it was when the plugin loaded, for the life of the server process. Correcting login.yml and reloading, as the issue's own retest did, could never help: the reload never reached the object holding the list. super.reloadSelf() is what actually reloads the config (plus the language file and @ConditionalOnConfig drift reporting); this module's own log line now runs after it instead of standing in for it. Also updates UltiLoginTest.reloadSelf to stub the now-reached static UltiToolsPlugin.getConfigManager() call site instead of NPE'ing, and adds reloadSelfReachesConfigManager as the direct regression guard at the bug's own site. Verified against 13-LEDGER-UltiLogin.md's "Recovery command diagnosis": instrument 1 already showed LoginConfig's own field binding works correctly once init() is actually invoked twice on the same instance; instrument 2 showed reloadSelf() invoked reloadConfigs() zero times before this fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4590bda30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public void invalidateSession(UUID playerUuid) { | ||
| String suffix = ":" + playerUuid; | ||
| sessions.keySet().removeIf(key -> key.endsWith(suffix)); |
There was a problem hiding this comment.
Revoke the active login state after administrative resets
When an administrator resets the password of a player who is currently online, this helper removes only the remembered sessions entries; it leaves loggedInPlayers set to true. LoginProtectionListener authorizes actions through isLoggedIn, not hasValidSession, so the existing connection remains fully authenticated with the old credentials until it disconnects, defeating password rotation as a response to a compromised account. The administrative reset paths should also force the affected online player back through authentication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real -- measured, not just plausible. LoginProtectionListener gates every in-game action on isLoggedIn(uuid), never on hasValidSession(player) (grep across the module's listener/commands/gui code: 12 call sites, all isLoggedIn, 0 use hasValidSession for authorization). Both resetPassword(UUID) and resetPassword(UUID, String) only called invalidateSession(), which ends the sessions map entry -- loggedInPlayers for an already-authenticated online player was left true, so they kept full access with the old (just-revoked) credentials until they happened to disconnect.
Fixed in commit ee28e39: both resetPassword overloads now also call a new forceReauthenticationIfOnline(UUID) helper, which flips loggedInPlayers to false for an online target (mirroring what unregister() already did) without replaying onPlayerJoin() -- that replay is a separate, already-fixed defect (13-06/D-08) because it re-runs the session auto-login check. Two new tests (ResetPassword.forcesReauthenticationForOnlinePlayerOnRandomReset / ...OnSpecificReset) reproduce the exact scenario you described (log in, then admin-reset while online) and were confirmed red against the pre-fix code before the fix landed.
| Player player = Bukkit.getPlayer(playerUuid); | ||
| if (player != null && player.isOnline()) { | ||
| loggedInPlayers.put(playerUuid, false); |
There was a problem hiding this comment.
Reinitialize the login timeout after unregistering online players
When an authenticated online player is unregistered, completeLogin has already removed their joinTimes entry, and this replacement for onPlayerJoin only flips the login flag. Consequently checkTimeouts() never sees this newly unauthenticated player, so the configured login timeout is no longer enforced while they remain connected. Start a fresh unauthenticated flow here without performing the now-invalid session auto-login check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real. completeLogin(Player) removes the player's joinTimes entry when they originally authenticate, and checkTimeouts() (the login-timeout enforcement, run every 20 ticks) iterates only over joinTimes.entrySet(). unregister()'s force-logout branch flipped loggedInPlayers to false but never re-added a joinTimes entry, so an online player forced back to unauthenticated by unregister() was invisible to checkTimeouts() and could stay connected indefinitely without ever being kicked for failing to log back in -- silently disabling the configured login timeout for exactly the player an administrator just flagged.
Fixed in commit ee28e39, alongside the resetPassword fix for your other comment: both now share a forceReauthenticationIfOnline(UUID) helper that sets loggedInPlayers.put(uuid, false) and joinTimes.put(uuid, System.currentTimeMillis()), matching what a genuine unauthenticated join would leave in joinTimes -- deliberately without replaying the rest of onPlayerJoin() (blind effect, teleport, session auto-login check), per your suggestion to start a fresh unauthenticated flow without the now-invalid session check. New test Unregister.reinitializesLoginTimeoutOnUnregister asserts joinTimes contains the player's UUID after unregister(), confirmed red before the fix.
…on guard Adds two tests that reproduce 13-REVIEW-UltiLogin.md CR-01: an in-flight UltiCloud panel magic-link request outlives unregister() and can log a deleted account back in via completePanelLogin. Both are currently red: - invalidateSessionCancelsPendingPanelRequest expects invalidateSession (the single entry point every credential-changing path already routes through) to also cancel a pending pendingPanelRequests entry and its pollingTasks task for that player. - refusesLoginForDeletedAccount expects completePanelLogin to refuse to complete a login when the account row no longer exists, as a second, independent layer of defense. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
…sion and guard completePanelLogin
CR-01 (13-REVIEW-UltiLogin.md): unregister() ended sessions map entries
but left an in-flight UltiCloud /panel magic-link request (and its
polling task) live. If the worker later confirmed the authentication,
completePanelLogin logged the just-deleted account back in -- the same
class of defect this phase closes, through a path invalidateSession was
never wired into.
- invalidateSession(UUID) -- the single entry point every
credential-changing path already routes through -- now also cancels
any pending pendingPanelRequests entry and its pollingTasks task for
that player.
- completePanelLogin refuses to complete a login when the account no
longer exists, as a second, independent layer of defense, and the
check now runs before completeLogin rather than after (also resolves
IN-01's ordering note).
Updates the pre-existing nullAccountPanelLogin test, which had encoded
the bug itself ("should still complete login" with no backing account)
as expected behavior.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
…isible across threads WR-01 (13-REVIEW-UltiLogin.md): the emailService field was a plain instance field with an unsynchronized lazy-init check, so two Bukkit scheduler threads calling getEmailService() concurrently on first use could race on the write. SimpleContainer.getBean(EmailService.class) returns the same singleton reference either way so this was benign in practice, but it is a latent data race a future non-idempotent lookup could turn into a real bug. Marking the field volatile is sufficient: it guarantees the second thread observes the first thread's write instead of independently re-running the lookup with stale visibility. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
WR-02 (13-REVIEW-UltiLogin.md): buildUpgradedServerFixture()'s mock Answer special-cased getConfigFile/getConfigFolder but not getResourceFolderPath -- a different, Lombok-generated public getter that ConfigManager.register(...) also reads. Left un-stubbed it fell through to RETURNS_DEFAULTS (null), and File(null, "config/login.yml") happened to resolve relative to the test JVM's working directory, making the fixture's correctness depend on no such directory existing there by accident. Stub it explicitly to the same temp config root as getConfigFolder. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
WR-03 (13-REVIEW-UltiLogin.md): login() and changePassword() compared Base64-encoded SHA-256 digests with String.equals, which short-circuits on the first mismatched character -- a textbook timing side-channel for a stored-hash comparison. Decode both to bytes and compare with MessageDigest.isEqual(byte[], byte[]), which always walks the full length of the shorter array. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
…timeouts Adds three tests reproducing the two inline Codex review comments on PR #18 (comment IDs 3944181256, 3944181260), currently red: - forcesReauthenticationForOnlinePlayerOnRandomReset and ...OnSpecificReset expect resetPassword(UUID) / resetPassword(UUID, String) to revoke isLoggedIn for a currently online, already authenticated player -- invalidateSession only ends the remembered session, and LoginProtectionListener authorizes actions through isLoggedIn, not hasValidSession, so the old connection stayed fully authorized until it happened to disconnect. - reinitializesLoginTimeoutOnUnregister expects unregister() to re-populate joinTimes for an online player it force-logs-out, so checkTimeouts() can still enforce the configured login timeout while they remain connected but unauthenticated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
…and unregister Addresses two inline Codex review comments on PR #18: - resetPassword(UUID) and resetPassword(UUID, String) previously only called invalidateSession(), which ends the remembered `sessions` entry. LoginProtectionListener authorizes in-game actions through isLoggedIn, not hasValidSession, so an already-authenticated online connection stayed fully authorized with the old credentials until it happened to disconnect -- defeating an administrative password reset issued in response to a compromised, currently-connected account. (comment 3944181256) - unregister()'s force-logout branch flipped loggedInPlayers but never re-added a joinTimes entry, which completeLogin had already removed when the player originally logged in. checkTimeouts() iterates joinTimes, so the newly-unauthenticated player was never kicked for failing to re-authenticate, silently disabling the configured login timeout while they remained connected. (comment 3944181260) Both are now handled by a shared forceReauthenticationIfOnline(UUID) helper, called from resetPassword's two overloads and from unregister in place of its previous inline duplicate. It deliberately does not replay onPlayerJoin() -- that replay is the separate, already-fixed 13-06/D-08 defect (it re-runs the session auto-login check). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee28e393d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| BukkitTask task = pollingTasks.remove(playerUuid); | ||
| if (task != null) { | ||
| task.cancel(); |
There was a problem hiding this comment.
Gate running panel polls after credential invalidation
When a reset or unregister occurs while the asynchronous polling task is already inside its HTTP request, cancelling the task only prevents future executions; the current invocation can continue after this method removes the pending request. The poll then finds no request ID and takes the fallback at startAuthPolling lines 1025-1029, which calls completeLogin(player) directly and bypasses the new registration check in completePanelLogin. Consequently, an already-running magic-link poll can re-authenticate an online player after password rotation or even after account deletion; the main-thread completion must require that the original request is still pending rather than treating its absence as authorization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed real, fixed.
BukkitTask#cancel() only prevents a scheduled task's future executions; it does not interrupt an invocation already inside its HTTP call. The old fallback looked up the pending request ID once, on the same call stack as the async HTTP fetch, and treated "not found" as authorization to call completeLogin(player) directly on the main thread -- bypassing completePanelLogin's registration check entirely. A poll started before an admin reset or unregister could therefore observe "completed" after the account changed and re-authenticate the player, undoing forceReauthenticationIfOnline in the same stroke.
Fix (eaa9027): extracted handlePanelPollCompleted(Player, boolean), invoked from the scheduled main-thread runnable, which looks up the pending request fresh at the point it actually runs rather than trusting a decision made earlier alongside the HTTP fetch. Every path that removes a pending request (cancelPendingPanelRequest via invalidateSession, or completePanelLogin's own cleanup) runs on that same main thread, so whichever removal happened first is guaranteed to be visible here. Absence of a pending request is no longer treated as authorization -- it is now a no-op in both cases it can mean (already completed elsewhere, or cancelled because the account changed).
Regression test (fe701c6, red before the fix): LoginServiceTest.StartAuthPollingRace#doesNotReauthenticateAfterUnregisterDuringInFlightPoll reproduces the exact race deterministically and single-threaded -- the mocked Bukkit scheduler runs both the async poll task and its main-thread completion synchronously, and the account is unregistered from inside the mocked HTTP call's answer, exactly the ordering this comment describes. (True multi-threading was not viable for this test: Mockito static mocks needed for UltiTools.getEnv()/SimpleHttpClient.get() are thread-confined to the thread that creates them and silently do not apply on a real MockBukkit worker thread -- verified empirically.) A companion test, completesLoginWhenNoRaceOccurs, guards that the uncontested completion path still logs the player in through completePanelLogin.
mvn -B verify: BUILD SUCCESS, 500 tests, 0 failures, coverage check passed.
…44418953) Adds a red test reproducing the P1 Codex flagged on the CR-01 fix commits: startAuthPolling's async poll looked up the pending request ID once, on the same call stack as the HTTP fetch, and treated "not found" as authorization to call completeLogin(player) directly -- bypassing every check completePanelLogin performs. BukkitTask#cancel() only prevents a scheduled task's future executions, not an invocation already inside its HTTP call, so a poll that started before an admin unregisters (or resets the password of) the account could still observe "completed" and re-authenticate the player, undoing forceReauthenticationIfOnline in the same stroke. doesNotReauthenticateAfterUnregisterDuringInFlightPoll reproduces the race deterministically and single-threaded: the mocked Bukkit scheduler runs both the async poll task and its main-thread completion synchronously, and the account is unregistered from inside the mocked HTTP call's answer -- exactly the ordering the report describes. Real multi-threading was not viable here: Mockito's static mocks (needed for UltiTools.getEnv() and SimpleHttpClient.get()) are thread-confined to whichever thread creates them, so they silently would not apply on a genuine MockBukkit worker thread. completesLoginWhenNoRaceOccurs is a companion regression guard proving the uncontested completion path still logs the player in through completePanelLogin -- this is also the first test in this class to exercise startAuthPolling at all; a prior comment on constructorResolvesFrameworkPlugin noted nothing reached it. Adds VaultAPI as a test-scope dependency (matching UltiChat, UltiTrade, UltiWorlds, etc.): mockStatic(UltiTools.class) must fully instrument UltiTools' class hierarchy, including getEconomy()'s Economy return type, and UltiTools-API's own VaultAPI dependency is provided-scope and therefore not transitive to this module's test classpath. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
…ore completing login Codex PR #18 review comment 3944418953: startAuthPolling's async poll looked up the pending request ID once, on the same call stack as the async HTTP fetch, then treated "no pending request found" as authorization to call completeLogin(player) directly on the main thread -- bypassing completePanelLogin's registration check entirely. BukkitTask#cancel() only prevents a scheduled task's future executions; it does not interrupt an invocation already inside its HTTP call, so a poll that started before an admin reset or unregister could still observe "completed" and re-authenticate the player after their credentials were revoked or their account deleted, undoing forceReauthenticationIfOnline in the same stroke. Extracts handlePanelPollCompleted(Player, boolean), invoked from the scheduled main-thread runnable, which looks up the pending request fresh at the point it actually runs rather than trusting a decision made earlier alongside the HTTP fetch. Every path that removes a pending request (cancelPendingPanelRequest via invalidateSession, or completePanelLogin's own cleanup) runs on that same main thread, so whichever removal happened first is guaranteed to be visible here. Absence of a pending request is no longer treated as authorization: either the login was already completed through completePanelLogin by another path (nothing left to do) or the request was cancelled because the account changed (must not log in) -- both cases are now a no-op instead of a silent completeLogin(player) bypass. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
…every credential change Real-machine finding F-L1 (13-uat-results.md #14, Laojun 2026-09-06): a real /changepassword reported success while the player stayed authenticated -- /mail inbox still worked, /recover and /login both said already logged in. invalidateSession only ended the remembered sessions entry; unlike unregister and resetPassword, changePassword never separately called forceReauthenticationIfOnline. invalidateSession is now the single entry point every credential-changing path can rely on: it forces re-authentication of an online player itself, so no caller can forget the step. Removed the now-redundant duplicate forceReauthenticationIfOnline calls from resetPassword (both overloads) and unregister. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d64deb5131
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public void invalidateSession(UUID playerUuid) { | ||
| String suffix = ":" + playerUuid; | ||
| sessions.keySet().removeIf(key -> key.endsWith(suffix)); | ||
| cancelPendingPanelRequest(playerUuid); |
There was a problem hiding this comment.
Fence panel requests that have not registered yet
When an administrator resets a password after /panel schedules its asynchronous worker but before requestPanelLink() inserts the request into pendingPanelRequests, this cancellation finds nothing; the worker then creates the request after invalidation, sends its link, and starts polling. Fresh evidence beyond the earlier in-flight-poll report is the scheduling gap in PanelCommand lines 56-79 before the insertion at requestPanelLink lines 890-892. Because password reset leaves the account registered, the new isRegistered guard also passes and completeLogin re-authenticates the revoked connection. Associate requests with an invalidation generation or otherwise prevent pre-reset work from publishing a request afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bcadfb5. Added a per-player invalidation generation (invalidationGenerations: Map<UUID, AtomicLong>, incremented by invalidateSession()); PanelCommand now captures loginService.getInvalidationGeneration(uuid) before scheduling the async worker, and the new requestPanelLink(Player, long) overload refuses to publish (and inserts nothing into pendingPanelRequests) when that captured generation no longer matches the current one.
| loggedInPlayers.put(playerUuid, false); | ||
| joinTimes.put(playerUuid, System.currentTimeMillis()); |
There was a problem hiding this comment.
Start the configured authentication flow after revocation
When an authenticated online player is reset or unregistered while GUI mode is enabled, this helper only flips the login flag and starts the timeout. LoginProtectionListener opens the login/register GUI from PlayerJoinEvent (lines 50-65) or after selected blocked chat/command interactions (lines 201-212), and neither occurs here, so the player is silently frozen by the action guards and can be kicked by the new timeout without ever being shown the configured credential screen. Start a no-session authentication flow here that presents the appropriate login/register GUI or prompt without replaying session auto-login.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bcadfb5. forceReauthenticationIfOnline() now also calls a new LoginService.presentCredentialPrompt(Player), which delegates to LoginProtectionListener's existing GUI-vs-text prompt branching (extracted into a static, parameterized presentCredentialPrompt(Player, UltiToolsPlugin, LoginService, Plugin) shared by both the listener's own sendLoginPrompt() and the service). The GUI branch is scheduled onto the main thread via Bukkit.getScheduler().runTask() since inventory APIs aren't thread-safe and the caller (an admin command) isn't guaranteed to already be there; it deliberately never consults hasValidSession(), so no session auto-login is replayed.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…on generation Own deep review of bcadfb5 (round 5, Codex quota exhausted): - requestPanelLink(Player, long)'s generation check and its pendingPanelRequests insert now run inside a synchronized block keyed on the player's own AtomicLong generation cell, the same cell invalidateSession() synchronizes its generation bump + cancellation on. This closes the narrow window a plain read-then-insert left open: either invalidateSession's bump-and-cancel runs fully before the check-and-insert (so the check sees it and refuses), or fully after (so the cancellation finds the entry the insert just made). Only the check+insert is inside the lock; the blocking HTTP call stays outside it. - Reworked RequestPanelLinkGenerationFence#refusesRequestCapturedBeforeInvalidation, which previously passed via the unrelated "API URL not configured" branch (UltiTools.getInstance() is null in this unit test) regardless of whether the fence check existed at all. Now stubs UltiTools.getEnv(), CommonUtils.getUltiToolsUUID(), and SimpleHttpClient.post() so the fence-free path would succeed, and asserts the fence's own outcome (isSuccess() false, error contains "invalidated", nothing inserted into pendingPanelRequests, SimpleHttpClient never invoked). Verified locally by disabling the fence check: the test fails with "Expecting value to be false but was true" on the isSuccess() assertion. - presentCredentialPrompt's text-prompt branch now dispatches through Bukkit.getScheduler().runTask(...) the same way the GUI branch already did, so a future async caller of invalidateSession cannot call a Bukkit player API off-thread through this path. Updated the affected LoginServiceTest/LoginProtectionListenerTest call sites to stub or capture the scheduler accordingly. - Fixed stale absolute line-number javadoc citations (LoginService.java:429, :481) to cite methods by name instead. - requestPanelLink(Player) (single-arg) is now package-private: nothing outside this class calls it, and its safety depends entirely on the caller staying on the same, uninterrupted call stack as the eventual publish -- a future public caller wrapping it in async scheduling would silently reintroduce the P1 this fence exists to close. env -u JAVA_TOOL_OPTIONS mvn -B -o clean verify: 504 tests, 0 failures, JaCoCo checks met. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…nd skip it while disabling Round 6 (13-REVIEW-UltiLogin.md, own review of 29ba589, Warning finding): both branches of LoginProtectionListener.presentCredentialPrompt dispatched unconditionally through Bukkit.getScheduler().runTask(bukkitPlugin, ...), which throws an unchecked exception when the owning plugin is disabled at call time -- and nothing upstream (LoginService.invalidateSession / forceReauthenticationIfOnline) caught it. Extract a dispatchOnMainThread helper used by both branches: run inline when already on the primary thread, schedule via runTask when off-thread and the plugin is still enabled, otherwise log a warning and skip rather than let the scheduler's exception propagate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d4a4f57f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (generationCell.get() != expectedGeneration) { | ||
| return new PanelLinkResult(false, null, | ||
| "Session was invalidated before the request completed"); |
There was a problem hiding this comment.
Re-check invalidation after the panel HTTP request
When a password reset or unregister occurs while SimpleHttpClient.post is blocked, this generation check has already passed and the request was already inserted, so invalidation removes the local entry but the worker can still return success; PanelCommand then sends the stale magic link and starts a new UUID-scoped poll. If the player creates a newer panel request, completion of the stale link can make that poll select the newer pending ID in findPendingRequestId and authenticate the revoked connection. Fresh evidence beyond the earlier pre-scheduling race is that the generation is checked here before the blocking POST, with no corresponding check before the successful result is consumed; retain the request ID/generation in the result and reject stale callbacks before sending or polling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dfcff51: requestPanelLink(Player, long) now re-checks the invalidation generation and pending-request membership under the same per-player lock immediately before returning a success result (after the POST returns), discarding the result instead of publishing it if either changed while the POST was in flight. PanelLinkResult now carries the requestId it was published under, and startAuthPolling/handlePanelPollCompleted/the poll's completion callback are all threaded with that exact id, so completion is resolved by "this poll's own request" rather than "whatever is pending now" for the player.
| // overload is also what the email-recovery flow delegates to | ||
| // (EmailVerificationService.resetPasswordAfterRecovery), so recovery inherits the | ||
| // invalidation (and the online re-authentication, F-L1) without a second call site. | ||
| invalidateSession(playerUuid); |
There was a problem hiding this comment.
Skip the credential prompt during successful recovery
When /recover <code> ... reaches this overload, invalidateSession now presents the login GUI or text prompt inline, but RecoverCommand immediately calls completeLogin(player) on success. Text-mode users therefore receive a contradictory instruction to log in immediately before being told they were automatically logged in, while GUI-mode users are left viewing a credential GUI that completeLogin never closes even though they are already authenticated. The recovery path needs to invalidate remembered/panel sessions without presenting the intermediate reauthentication prompt, or complete the recovery login as one operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dfcff51: added invalidateSession(UUID, boolean presentPrompt) and a package-private resetPasswordForRecovery(UUID, String) that EmailVerificationService.resetPasswordAfterRecovery now calls instead of the public, prompt-presenting resetPassword(UUID, String) -- recovery revokes remembered/panel sessions and bumps the generation without showing the credential prompt, since RecoverCommand immediately calls completeLogin(player) itself. completeLogin now also closes an already-open LoginGUIPage/RegisterGUIPage if one is showing. Unregister/reset/changePassword are unaffected and still present the prompt.
…eep recovery free of the re-login prompt Codex PR #18 round 7, two findings against head 1d4a4f5: - requestPanelLink(Player, long) checked the invalidation generation and inserted into pendingPanelRequests before the blocking HTTP POST, but never re-checked afterward -- an invalidateSession(...) landing during the (unbounded) POST call removed the local entry and bumped the generation, yet the method still returned success, and startAuthPolling/handlePanelPollCompleted resolved completion by scanning for "whatever is pending now" rather than the exact request id, so a stale completion could authenticate through an unrelated, newer request for the same player. Fixed by re-checking generation + pending-request membership under the same per-player lock immediately before returning success, carrying the request id through PanelLinkResult, and threading that id through startAuthPolling -> handlePanelPollCompleted -> completePanelLogin so completion is always resolved by the exact id a poll was started for. - Successful email recovery invalidated the session (which now presents the credential prompt inline) and then immediately called completeLogin(player), showing "log in now" right before "you are logged in" in text mode, or leaving a credential GUI open in GUI mode. Added invalidateSession(UUID, boolean)/ resetPasswordForRecovery(UUID, String) so the recovery path can skip the prompt, and completeLogin now closes an already-open LoginGUIPage/RegisterGUIPage. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfcff517f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| loggedInPlayers.put(playerUuid, false); | ||
| joinTimes.put(playerUuid, System.currentTimeMillis()); |
There was a problem hiding this comment.
Reapply unauthenticated protections after revocation
When an online player's password is reset or changed, or their account is unregistered, this helper only clears the login flag and restarts the timeout. If blind-effect or spawn-location.enabled is configured, the sole code applying those protections is the join flow at onPlayerJoin (lines 615-635), so the revoked client remains fully sighted at its current potentially sensitive location despite now being unauthenticated. Reinitialize the configured no-session protections here while still avoiding the session auto-login check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8945150: extracted the join flow's blind-effect/spawn-teleport logic into a shared LoginService.applyNoSessionProtections(Player), dispatched on the main thread via LoginProtectionListener.dispatchOnMainThread (widened to public), and called from forceReauthenticationIfOnline for every revocation path (unregister, both admin resetPassword overloads, changePassword) but not the silent recovery path; completeLogin already restores the recorded location and removes the blindness effect on the player's next successful login.
…s credentials are revoked Codex PR #18 round 8, thread 3946414499: forceReauthenticationIfOnline() only flipped loggedInPlayers/joinTimes when revoking an online player's session (account deletion, an administrative password reset, or a self-service password change). blind-effect and spawn-location.enabled are only ever applied by onPlayerJoin(), which never runs again for an already-connected player, so a revoked client stayed fully sighted at its current, potentially sensitive location despite now being unauthenticated. Extracted the blind-effect/spawn-teleport logic onPlayerJoin() already applied into a shared LoginService.applyNoSessionProtections(Player), dispatched onto the main thread through LoginProtectionListener.dispatchOnMainThread (widened from private to public for this cross-package call, matching the existing presentCredentialPrompt() delegation pattern). forceReauthenticationIfOnline() now calls it whenever presentPrompt is true -- unregister, both admin resetPassword overloads, and changePassword -- but not for the silent recovery path (resetPasswordForRecovery), which re-authenticates the player itself immediately afterward. A spawn teleport records the player's current location into originalLocations first (only if not already present), so completeLogin() on the player's next successful login restores it exactly as it already does for the ordinary join-time protections. Tests (RED verified by reverting the source changes, GREEN after restoring): - ForceReauthenticationCredentialPrompt#reappliesNoSessionProtectionsWhenAnOnlinePlayerIsRevoked - ForceReauthenticationCredentialPrompt#doesNotApplyNoSessionProtectionsOnSilentRecovery - ForceReauthenticationCredentialPrompt#clearsReappliedProtectionsAfterSuccessfulReLogin Full suite: mvn -B -o clean verify -> BUILD SUCCESS, Tests run: 515, Failures: 0, Errors: 0, Skipped: 0, JaCoCo coverage gate met. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8945150bc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Run async to avoid blocking the main thread (HTTP call) | ||
| Bukkit.getScheduler().runTaskAsynchronously(bukkitPlugin, () -> { | ||
| LoginService.PanelLinkResult result = loginService.requestPanelLink(player); | ||
| LoginService.PanelLinkResult result = loginService.requestPanelLink(player, invalidationGeneration); |
There was a problem hiding this comment.
Revalidate the panel request in the main-thread callback
When a password reset or unregister occurs after requestPanelLink performs its final generation check but before the queued main-thread callback runs, this successful result is still consumed: the revoked player receives the stale magic link and a new polling task starts after invalidation already cancelled the old state. Fresh evidence beyond the earlier report is the asynchronous handoff at lines 67-88, where the callback never rechecks the captured generation or exact pending request before publishing the URL; repeat that validation in the callback before sending the link or starting polling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 30228ce: added LoginService.isPanelRequestCurrent(UUID, String, long), which re-checks the invalidation generation and pendingPanelRequests under the same per-player lock invalidateSession synchronizes on, and PanelCommand's result-delivery callback now calls it before acting on a successful result, discarding (no link, no poll, plain failure message) if it comes back false.
| applyNoSessionProtections(player); | ||
| presentCredentialPrompt(player); |
There was a problem hiding this comment.
Avoid overlapping credential GUIs after unregister
In GUI mode, if an online registered-but-unauthenticated player already has LoginGUIPage open when an administrator unregisters them, this prompt opens RegisterGUIPage over it. Closing the old login GUI schedules another login GUI in LoginGUIPage.onClose because the player remains logged out, while closing that replacement schedules another register GUI in RegisterGUIPage.onClose because the account is gone, causing the two screens to replace each other every 10 ticks until the player is kicked. Transition the existing credential GUI without activating its reopen hook, or make each close hook verify that its GUI still matches the player's registration state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 30228ce: LoginGUIPage/RegisterGUIPage's onClose hooks now check isRegistered (not just isLoggedIn) before rescheduling, and LoginProtectionListener.presentCredentialPrompt marks the player as mid-transition (LoginService.begin/endCredentialGuiTransition) while it swaps GUIs, so the GUI being replaced skips its own reopen entirely instead of fighting the new one.
…tion credential GUIs without the reopen loop Round 9 Codex triage against PR #18 head 8945150, two P2 findings. Thread 3946574845: requestPanelLink(Player, long)'s post-POST re-check (round 7) only closed the race up to the moment that method returned -- PanelCommand's result-delivery callback is itself scheduled via runTask(...) after that, so an invalidation landing in that final gap could still hand a revoked player a live magic link and start a new poll for it. Added LoginService.isPanelRequestCurrent( UUID, String, long), which re-checks the generation and the pending-request entry under the same per-player lock invalidateSession synchronizes on, and PanelCommand now calls it at the top of the callback: a stale result is discarded outright (no link sent, no poll started) rather than acted on. Thread 3946574852: an online, registered-but-unauthenticated player with LoginGUIPage open, unregistered by an admin, had the revocation prompt open RegisterGUIPage over it -- which implicitly closes the currently-open inventory, firing LoginGUIPage's own onClose reopen hook. That hook only checked isLoggedIn, not isRegistered, so it rescheduled a login GUI for an account that no longer existed, and the two pages fought each other every 10 ticks until the player was kicked. Fixed both halves: LoginGUIPage/RegisterGUIPage's close hooks now verify their own GUI still matches the player's current registration state (outer check and the delayed re-check) before reopening, and LoginProtectionListener. presentCredentialPrompt now marks the player as mid-transition (LoginService. begin/endCredentialGuiTransition) around the GUI swap so the hook being replaced skips its own reopen logic entirely rather than racing the new GUI. Tests: PanelCommandTest#discardsResultWhenInvalidatedBeforeCallbackRuns (RED: TooManyActualInvocations on spigot.sendMessage -- the stale result's link was sent anyway); LoginServiceTest$CredentialGuiTransition#unregisterWhileLoginGuiOpenOpensExactlyOneRegisterGuiWithoutPingPong (RED: NeverWantedButInvoked on runTaskLater -- the login GUI rescheduled itself after being replaced by RegisterGUIPage); #closingLoginGuiWhileStillRegisteredAndUnauthenticatedStillReopensIt is a green regression guard for the unaffected normal case. Full suite: 518 tests, 0 failures, JaCoCo coverage gate met. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30228ce164
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Bukkit.getScheduler().runTask(bukkitPlugin, | ||
| () -> handlePanelPollCompleted(player, requestId, finalIsServerOwner)); |
There was a problem hiding this comment.
Bind remote poll results to the exact request
The poll request is still scoped only by playerUuid, and the response contains no request ID, so this callback assigns its captured requestId to any completed response for that player. After invalidation cancels request A, /panel remains allowed while unauthenticated, so the player can create request B; if the still-live remote link A is then completed, B's UUID-scoped poll calls completePanelLogin(B) and re-authenticates the revoked connection. Fresh evidence beyond the earlier exact-ID finding is that threading the local ID here does not correlate it with the remote response; the poll URL at line 1415 and parsed response never identify which request completed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and this one is real -- confirmed by reading requestPanelLink's create call and startAuthPolling's poll call side by side.
What the current remote contract actually sends/returns. The create call (POST {api-url}/auth/magic-link) sends requestId, code, playerUuid, playerName, serverUuid, and the response only ever carries url (LoginService.java:~1424). The poll call (GET {api-url}/auth/magic-link/poll?playerUuid=...) is scoped by playerUuid alone -- no requestId in the query string -- and its response (LoginService.java:~1555) is parsed for status and is_server_owner only; there is no field anywhere in that response that identifies which magic-link request completed. So requestId never round-trips through the worker at all -- it is a purely local value this module invents, tracks, and threads through its own callbacks, and the worker has no way to know it exists, let alone echo it back or scope a poll response by it.
What this module can, and cannot, verify from that. Every round-4-through-9 fix (generation fencing before publish, the atomic check-and-insert, the post-POST re-check, isPanelRequestCurrent on the main-thread callback, request-id-keyed local pendingPanelRequests lookups) closes gaps in this module's own bookkeeping -- races between a local invalidation and a local publish/poll/callback. None of them can close the gap this thread flags, because it isn't a local race: it's that the poll response itself carries no way to distinguish "request A completed" from "request B completed" for the same player. A locally-cancelled request A's underlying remote magic link is never told to stop being valid, so if a player creates a new request B after A is invalidated, B's own (correctly-scoped, correctly-request-id-keyed-locally) poll can still observe A's completion and authenticate through it, because from the worker's perspective there is only ever "is there a completed status for this playerUuid", not "is request B specifically completed".
Why this isn't fixed here. Closing it for real needs the create call to return (and the worker to persist) a per-request id or nonce, the poll endpoint to accept and echo that same id back in its response (or be scoped by it in the query), and a worker-side revoke of a pending request when this module's invalidateSession fires -- all three are protocol/worker-side changes, not something UltiLogin can implement unilaterally against the contract as it exists today.
Tracked as a cross-repository protocol change:
- UltiLogin: Panel login: correlate the poll result with the exact request id once the worker echoes it #19
- ultipanel-api-worker: UltiKits/ultipanel-api-worker#36
Useful? React with 👍 / 👎.
| if (player.isOnline() && loginService.isRegistered(uuid) && !loginService.isLoggedIn(uuid)) { | ||
| new LoginGUIPage(player, plugin, loginService).open(); |
There was a problem hiding this comment.
Suppress already-queued credential GUI reopen tasks
When a registered, logged-out player closes LoginGUIPage, this schedules a reopen for 10 ticks later; if an administrator resets that player's password during the delay, the new revocation prompt opens another login GUI first. The queued callback then opens a second GUI over it, whose close hook schedules another callback, producing a recurring replacement every 10 ticks. Fresh evidence beyond the earlier GUI-transition finding is that the transition marker only covers the synchronous prompt-driven close, while this delayed callback checks neither the marker nor whether a credential GUI is already open.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 909362f: presentCredentialPrompt now cancels any queued reopen before opening its GUI, and the queued reopen's own runnable re-checks the transition marker and a new per-player "credential GUI open" tracker fresh at fire time, so a reopen queued before a credential change can no longer stack a second GUI over the one the prompt just opened (both LoginGUIPage and RegisterGUIPage; new regression tests in LoginServiceTest$CredentialGuiTransition).
Codex PR #18 round 10 (thread 3946842965): a registered, logged-out player closing LoginGUIPage schedules a reopen 10 ticks later. An admin password reset (or self-service change) landing in that window doesn't alter isRegistered/isLoggedIn, so the queued reopen's state check alone couldn't detect that a credential change had already opened a fresh credential GUI in the meantime -- it opened a second GUI on top of the one the reset's own prompt just opened, whose own onClose then queued another reopen, replacing the GUI every 10 ticks indefinitely. Two independent defenses close this, symmetrically in both LoginGUIPage and RegisterGUIPage: - presentCredentialPrompt now cancels any reopen task queued for the player (LoginService#cancelPendingCredentialGuiReopen) before opening its own GUI, so a stale queued reopen can no longer fire afterward. - The queued reopen's own runnable re-checks isCredentialGuiTransitioning and a new isCredentialGuiOpen tracker (set by onOpen, cleared by onClose) fresh at fire time, instead of trusting state read 10 ticks earlier, so it refuses to stack a second GUI even if cancellation somehow didn't take. registerCredentialGuiReopenTask/clearCredentialGuiReopenTask track the queued task per player; the self-clear is unconditional rather than keyed on task identity, since a cancelled BukkitTask is guaranteed by the scheduler contract to never fire, so at most one registered task per player can ever reach it. Tests: LoginServiceTest$CredentialGuiTransition adminResetDuringQueuedReopenDelayCancelsTheQueuedReopenAndOpensExactlyOneGui (RED before the fix: the queued task's runnable reached LoginGUIPage's constructor a second time) and queuedReopenStillFiresNormallyAndSelfClearsItsBookkeeping (regression guard for the unmolested reopen path and the self-clear bookkeeping). Full suite: 520 tests, 0 failures, JaCoCo coverage gate met. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 909362f15e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Complete login on main thread, resolving by the exact request id this poll was | ||
| // started for -- see handlePanelPollCompleted's javadoc for why. | ||
| Bukkit.getScheduler().runTask(bukkitPlugin, | ||
| () -> handlePanelPollCompleted(player, requestId, finalIsServerOwner)); |
There was a problem hiding this comment.
Prevent stale polls from cancelling their replacements
When poll A is cancelled during an in-flight HTTP call and the player subsequently starts poll B, A can still return completed; the UUID-scoped pollingTasks.remove(uuid) immediately above this callback then retrieves and cancels B rather than A. This exact-request callback correctly refuses to complete the removed request A, but B is now permanently stopped and its valid magic link cannot complete unless the player starts over, so task removal needs to be conditional on the task/request that is actually finishing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ee795ba: pollingTasks is now keyed by request id (not player UUID), with a separate UUID -> requestId index (currentPollingRequestId) for the unconditional "stop whatever is active for this player" callers (cancelPendingPanelRequest, onPlayerQuit) — each poll's own cleanup now only ever touches its own entry, so a stale poll A's completion can never remove or cancel a superseding poll B's task.
pollingTasks was keyed by player UUID, so an older poll (A) whose own scheduled task was already cancelled and replaced by a newer poll (B) -- because the player started a fresh /panel request while A's HTTP call was still in flight -- would, on its own stale "completed" observation, blindly remove and cancel whatever task the map currently held for that player: B's, not its own. handlePanelPollCompleted correctly refused to complete A's own (already superseded) request, but B's task was killed anyway, leaving B's still-valid magic link unable to ever complete without the player starting over. Key pollingTasks by request id instead of player UUID, with a separate UUID -> requestId index (currentPollingRequestId) for the unconditional "stop whatever is active for this player" callers (cancelPendingPanelRequest, onPlayerQuit). Each poll's own cleanup now only ever touches its own entry, regardless of which HTTP call returns first. Codex PR #18 thread 3947189541. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee795ba250
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dispatchOnMainThread(player, plugin, bukkitPlugin, () -> { | ||
| if (loginService.isRegistered(player.getUniqueId())) { |
There was a problem hiding this comment.
Recheck login state before sending queued text prompts
In command mode, AsyncPlayerChatEvent reaches this branch off the main thread, so the new dispatcher queues the prompt for a later tick. If the player is force-logged-in or otherwise authenticates before that callback runs, it still sends the login/register instruction because, unlike the GUI branch, it checks neither player.isOnline() nor isLoggedIn(...) at execution time. Revalidate those states inside the queued callback to avoid contradictory prompts after successful authentication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b95caf: the queued text-prompt callback now re-checks player.isOnline() and loginService.isLoggedIn(...) at execution time, mirroring the GUI branch, so it no longer sends the login/register instruction to a player who authenticated while the callback was queued for the next tick.
Codex PR #18 thread 3947572910 (P3): AsyncPlayerChatEvent fires off the main thread, so the non-GUI text-prompt branch of LoginProtectionListener.presentCredentialPrompt always queues its message send via dispatchOnMainThread for a later tick. Unlike the GUI branch, the queued callback sent unconditionally, with no re-check of player.isOnline()/loginService.isLoggedIn(...) at execution time -- a player force-logged-in (or otherwise authenticated) in the gap between queuing and that tick still received the login/register instruction. The queued text-prompt callback now re-checks exactly what the GUI branch already checks before sending anything. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b95cafd6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // /ul reload UltiLogin ran or what the file said afterward. super.reloadSelf() is what | ||
| // actually reloads the config (plus the language file and @ConditionalOnConfig drift | ||
| // reporting); this module's own log line stays after it as user-facing confirmation. | ||
| super.reloadSelf(); |
There was a problem hiding this comment.
Preserve pending spawn restoration across reloads
When an unauthenticated player has already been teleported because spawn-location.enabled was true, reloading after changing that setting to false now updates the live LoginConfig; however, LoginService.completeLogin() restores originalLocations only when the newly reloaded setting is still true (lines 947-954). The player therefore remains stranded at the authentication spawn and the saved location is discarded after login. Track whether each player was actually teleported, or reconcile/restore affected players before applying the new configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9509195: completeLogin() now restores the saved location whenever an originalLocations entry exists for the player, independent of the current spawn-location.enabled value — the entry's presence is the record that a teleport happened, not the live config. Regression test: LoginServiceTest$CompleteLoginWithSpawn#restoresSavedLocationRegardlessOfCurrentSpawnSetting (RED confirmed before the fix, GREEN after; full suite 526/526).
…he current spawn setting completeLogin() gated the originalLocations restore on config.isSpawnLocationEnabled() at *restore* time instead of on the entry's presence. If an unauthenticated player was teleported to spawn while the setting was true, then the setting was flipped to false (config reload) before the player logged in, the player was left stranded at spawn and their saved location was discarded on the removal that followed. The saved-location entry itself is the record that a teleport happened -- restore whenever it exists. Codex PR #18 round 13, thread 3947908093. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Issue closure
Closes #14
Closes #15
Closes #13
(#12 was already closed separately, by evidence, during plan 13-03's reconfirmation pass — see
below. This pull request does not claim to close it a second time.)
Summary
Two Phase 13 module-defect-closure plans landed on this branch: session invalidation and a
compile-blocking dependency fix (13-06), then the root cause of password recovery being
unreachable on upgraded servers (13-13). Both are described in full below, plainly, because
#14 and #13 are public issues whose bodies already give the mechanism.
#14 — No session invalidation on unregister / password change / recovery
LoginServicenever ended a player's existing session when their account was deleted, theirpassword was reset by an administrator, they changed their own password, or they recovered
their account by email. Worse,
unregister(UUID)replayedonPlayerJoin()after deleting theaccount row, which found the still-live session and logged the just-deleted account straight
back in.
Added
LoginService.invalidateSession(UUID)— removes every session-map entry whose key endswith
:<playerUuid>, so a session opened from a different address than the caller's own is alsoremoved (an administrator ending a session they are not connected from needs exactly this).
Wired into all four credential-affecting paths on their success branch only:
unregister(UUID)(with the
onPlayerJoin()replay removed entirely, not merely reordered — the replay itself wasthe defect),
resetPassword(UUID)(administrator reset),resetPassword(UUID, String)(self-service reset, and what email recovery delegates to), and
changePassword(UUID, String, String).Proven by seven tests: one per call site, each red when its own
invalidateSessioncall isremoved, plus an end-to-end deletion test (log in → delete account → session check false →
rejoining does not auto-login) and an end-to-end recovery test driving the real
EmailVerificationService.resetPasswordAfterRecoverydelegation chain, plus a cross-addresssuffix-match test pinning that invalidation is not scoped to the caller's own current address.
#15 —
/regs <email>always crashesEmailVerificationServicedepended on the framework'sContextHolderclass, which theframework deleted outright on 2026-09-04 (issue 390) rather than merely leaving uninitialised —
so the module could not even compile against the current framework snapshot, let alone run.
getEmailService()now resolvesEmailServiceviaplugin.getContext().getBean(EmailService.class)— the module's own IoC container, whose lookup falls through to the shared core container. The
test's
ContextHolderstub and its reflection injection of the privateemailServicefield wereboth removed; both had independently hidden the real defect by never exercising the actual lookup
path.
#13 — Password recovery unreachable on upgraded servers
The issue's own opening theory — that an existing
login.yml's shorterallowed-commandslistwas the whole story — was falsified by the issue's own retest: correcting the file to the
current default and reloading did not make
/recoverreachable. Every step of the read chain(
LoginConfig→@ConfigEntry→ConfigManager.reloadConfigs→LoginService.isCommandAllowed)looked correct on paper.
The actual cause, found by measurement rather than more source-reading:
UltiLogin.reloadSelf()overrides
UltiToolsPlugin.reloadSelf()without callingsuper.reloadSelf()./ul reload UltiLoginlogged "UltiLogin 配置已重载!" ("UltiLogin config reloaded!") and reported success,but never actually called
ConfigManager.reloadConfigs(this)— the only thing that re-readslogin.ymlinto a runningLoginConfig.allowedCommands(and every other@ConfigEntryfield on
LoginConfig) was frozen at whatever it was when the plugin loaded, for the life of theserver process, regardless of how many times the file was corrected and reloaded. Two isolated
measurements confirmed this before any fix was written:
LoginConfig's own field binding updatescorrectly when
init()is actually invoked a second time on the same instance (proven directly),and
UltiLogin.reloadSelf(), as written, invokedConfigManager.reloadConfigs()zero times(also proven directly, by driving the real method body).
Fix:
UltiLogin.reloadSelf()now callssuper.reloadSelf()before its own log line. Proven bythree tests exercising the real
ConfigManager, the realLoginConfigbinding, and the realUltiLogin.reloadSelf()method body against a stored configuration in the shape an upgradedserver actually has (missing
regs/recover): the recovery command becomes reachable after acorrected file plus a reload, a command outside the permitted set stays refused (the fix does not
widen the gate), and a direct assertion on the in-memory list the running plugin holds matches
what the file says after the reload. Plus a direct regression test at the bug's own site
(
UltiLogin.reloadSelf()now reachesConfigManager.reloadConfigs(plugin)).#12 — closed separately, by evidence
Already closed during plan 13-03's reconfirmation, before this pull request's own work began:
disassembling
LoginProtectionListener.classagainst the current framework jar shows bothInventoryView.getTitle()call sites compiling asinvokeinterface(zeroinvokevirtual) —Phase 12's recompilation against
paper-api:1.21.11-R0.1-SNAPSHOT(whereInventoryViewis aninterface) already removed the
IncompatibleClassChangeErrorcondition this issue reported. Nocode was added for it in either plan on this branch.
Review fixes
Applied after the GSD Phase 13 deep code review (
13-REVIEW-UltiLogin.md) and after triagingCodex's two inline PR comments. RED/GREEN commit pairs per
workflow.tdd_mode.CR-01 — deleted/reset account could be logged back in via the panel magic-link flow
unregister(UUID)endedsessionsentries but left an in-flight UltiCloud/panelmagic-linkrequest (and its polling task) live. If the worker later confirmed that authentication,
completePanelLoginlogged the just-deleted account back in — the same class of defect #14closed, through a path
invalidateSessionwas never wired into.invalidateSession(UUID)— the single entry point every credential-changing path already routesthrough — now also cancels any pending panel-link request and its polling task for that player.
completePanelLoginadditionally refuses to complete a login when the account no longer exists,as a second, independent layer of defense, checked before the login is granted rather than after.
WR-01/WR-02/WR-03 — hardening and test-fixture findings
EmailVerificationService's lazily-cachedEmailServicefield is nowvolatile(latent,benign data race on first concurrent lookup).
getResourceFolderPath()explicitly, so it nolonger depends on the test JVM's working directory happening not to contain a colliding path.
login()andchangePassword()now useMessageDigest.isEqual(byte[], byte[])instead ofString.equals, closing a timingside-channel in the authentication path.
Codex PR review — two confirmed, both fixed
resetPassword, either overload) ended the rememberedsession but never revoked
loggedInPlayersfor an already-authenticated online target —LoginProtectionListenerauthorizes onisLoggedIn, nothasValidSession, so the oldconnection stayed fully authorized with the old credentials until it happened to disconnect.
unregister()'s force-logout branch never re-populatedjoinTimesfor the online player itjust force-logged-out, so
checkTimeouts()(which only iteratesjoinTimes) could neverenforce the configured login timeout on them while they stayed connected.
Both are now handled by a shared
forceReauthenticationIfOnline(UUID)helper, called (at thetime) from both
resetPasswordoverloads and fromunregisterin place of its previous inlineduplicate. Deliberately does not replay
onPlayerJoin()— that replay is the separate,already-fixed 13-06/D-08 defect. Superseded below by the F-L1 fix, which folds this helper's
call into
invalidateSession(UUID)itself so no caller can omit it.Codex PR review round 2 — one confirmed, fixed
The panel magic-link poll's completion fallback treated "no pending request found" as
authorization to log the player in directly, bypassing
completePanelLogin's registrationcheck entirely.
BukkitTask#cancel()does not stop an invocation already inside its HTTP call,so a poll started before an admin reset or unregister could still re-authenticate the player
after the account changed, undoing the round-1
forceReauthenticationIfOnlinefix above in thesame stroke.
Fixed by re-checking the pending request fresh, on the main thread, at the point login is
actually about to be granted, instead of trusting a decision made earlier alongside the async
HTTP fetch — see
LoginService.handlePanelPollCompleted. Proven RED-then-GREEN byLoginServiceTest.StartAuthPollingRace#doesNotReauthenticateAfterUnregisterDuringInFlightPoll,which reproduces the exact race deterministically and single-threaded (real multi-threading was
not viable: Mockito's static mocks are thread-confined and silently would not have applied on a
genuine MockBukkit worker thread).
Real-machine finding F-L1 —
changePasswordstill left the online player authenticatedFound by Laojun's real-machine acceptance session (2026-09-06), after every commit above landed on
this branch: a real
/changepassword, reproduced twice, reported "密码修改成功!" while the playerstayed fully authenticated —
/mail inboxstill worked, and/recover//loginboth said alreadylogged in.
changePassword(UUID, String, String)was the one credential-changing path that calledinvalidateSession(UUID)without a matchingforceReauthenticationIfOnline(UUID)call — theCodex PR review round-1 fix above added that second call to
resetPassword(both overloads) andunregister, but not tochangePassword.Rather than add the missing call at that one remaining site,
invalidateSession(UUID)now callsforceReauthenticationIfOnline(UUID)itself, so it is a complete, single entry point everycredential-changing path can rely on without a second call. The now-redundant duplicate calls at
resetPassword(both overloads) andunregisterwere removed. Proven RED-then-GREEN byLoginServiceTest.ChangePassword#forcesReauthenticationForOnlinePlayerOnChangePassword, whichobserved RED as
Expecting value to be false but was trueonisLoggedIn(playerUuid)before thefix, and is GREEN after.
Codex PR review rounds 4-7 -- panel-request invalidation races and the credential prompt
Four further review rounds against this branch, each fixing one confirmed finding (own reviews
substituted for Codex when its quota was exhausted, per the phase's orchestrator policy):
/panel's async worker startedbut before it published its request found nothing to cancel, letting a now-stale request
through; and revoking an online player's login state left them frozen with no way back to the
credential screen. Fixed with a per-player invalidation-generation counter
/panelcapturesbefore scheduling its worker (
requestPanelLink(Player, long)refuses to publish a requestwhose captured generation has since changed), and by having
forceReauthenticationIfOnlinepresent the same login/register prompt
LoginProtectionListeneralready shows after a blockedaction.
pendingPanelRequestsinsertwere two separate steps, not atomic with each other -- closed by synchronizing both, and
invalidateSession's own bump-and-cancel, on the same per-player lock. Also closed a latentthread-affinity gap (the prompt's text branch called Bukkit APIs off-thread while the GUI
branch was already main-thread-safe) and two smaller javadoc/test-quality findings.
Bukkit.getScheduler().runTask(...)call could throw if the owning plugin was mid-disable; extracted a
dispatchOnMainThreadhelper that runs inline on the main thread, schedules normally off-thread while the plugin is
enabled, and skips with a logged warning rather than propagating the scheduler's exception
while the plugin is disabling.
requestPanelLinkstill returned success for a request whose invalidation raced its own blocking HTTP POST --
fixed by re-checking the generation and pending-request membership under the same lock
immediately before returning, and by threading the exact request id through
PanelLinkResult/startAuthPolling/handlePanelPollCompletedso poll completion is neverresolved against "whatever is currently pending" for the player. Second, a successful email
recovery showed the credential prompt (or left a
LoginGUIPageopen) an instant beforecompleteLoginran -- fixed withinvalidateSession(UUID, boolean)and a recovery-onlyresetPasswordForRecovery(UUID, String)that skips the prompt, plus acompleteLoginchangethat closes an already-open credential GUI.
forceReauthenticationIfOnlineonly flipped the login flag and restartedthe join timeout for a revoked online player --
blind-effectandspawn-location.enabledareonly ever applied by
onPlayerJoin, which never runs again for an already-connected player, soa revoked client stayed fully sighted at its current, potentially sensitive location despite
now being unauthenticated. Fixed by extracting that join-time logic into a shared
applyNoSessionProtections(Player), dispatched through the round-6dispatchOnMainThreadhelper (widened to public) and called from
forceReauthenticationIfOnlinefor everyrevocation path but not the silent recovery path; a spawn teleport records the player's
current location first so
completeLoginrestores it on their next successful login.Full outcome table, evidence, and the Codex reply text:
13-REVIEW-UltiLogin.md's## Fix outcomessection (local planning artifact, not tracked in this repository).Test plan
mvn -B verify— 515 tests, 0 failures, 0 errors, jacoco coverage check met,BUILD SUCCESS(501 at round 3/F-L1; rounds 4-8 above added the rest)the correct reason in each case (transcripts in the phase's local evidence ledger)
Gates
every behavioral change, deviation-rule discipline applied and documented.
(standing deferred item, tracked in the framework's own
CLAUDE.md) — this is a standingscope gap, not a quota skip for this specific pull request.
after every Phase 13 pull request's machine-verifiable gates are green (plan D-11).
(
gh api repos/UltiKits/UltiLogin/branches/master/protection→404 Branch not protected,confirmed this session) — its
maven-ci.ymlresult is the de facto gate rather than amechanically enforced one.
This pull request is not merged by this plan. All Phase 13 module pull requests merge together
after the batched acceptance session (plan D-12).
Known gap tracked cross-repo
Codex PR review round 10 (thread 3946842960) confirmed a residual protocol gap in the panel
magic-link flow: the poll endpoint is scoped only by
playerUuidand its response carries norequest id, so a request cancelled by a local invalidation cannot be distinguished, on the wire,
from a newer request for the same player. Closing it requires the UltiPanel worker to return and
accept a per-request id and revoke pending requests on invalidation -- out of scope for this
module alone. Tracked as:
🤖 Generated with Claude Code
https://claude.ai/code/session_01V1b7Av7gngbARNsU1b5htv