Repository navigation
fix(users): re-check the session user's status on every request - #38469
jcastro-dotcms wants to merge 20 commits into
Conversation
Resolve the user of a session through a new SessionUserResolver that reads the current user record (cache-backed) instead of trusting the copy stored at login. A user that is no longer active, or no longer exists, resolves to no user and the session is signed out, so REST, the back-end UI, DWR and the Velocity tools all see the change on the next request, on any node. The existing ALLOW_INACTIVE_ACCOUNTS_TO_LOGIN switch keeps inactive accounts usable when it is on. Saving a user as inactive, and deleting a user, end that user's tracked sessions on the node after commit and rotate the user's login token, so a persistent login cookie issued before the change is rejected. The Maintenance session tools rotate the login tokens of the users whose sessions they end, and the session listing keeps showing sessions whose user is now inactive. Sessions are registered with the SessionMonitor as soon as the user id is stored, so every login path is tracked before the next request. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
…er-status # Conflicts: # dotcms-integration/src/test/java/com/dotcms/rest/WebResourceIntegrationTest.java
|
Claude finished @jcastro-dotcms's task in 7m 21s —— View job Code Review —
|
The session listing resolved each session's user through the regular session user lookup, which drops the user ids of a session whose user is no longer usable. The row then fell back to the anonymous user and a later kill of that session had no user id left to rotate login tokens for. The listing now reads the stored user id and loads the record as it is, so the row names the right user and the session keeps its ids. Two integration tests cover the listing of a session whose user was made inactive and a user id set as a request attribute. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
A session that was signed out (its user id removed) and then invalidated
stayed in the SessionMonitor registry until its entry expired, because the
monitor only forgot sessions that still carried a user id when destroyed,
and the Maintenance kill tools invalidated sessions without going through
the monitor. Listing sessions then failed with 400 ("Session already
invalidated") for as long as the stale entry lived.
The monitor now forgets a session when it is destroyed, whatever its
attributes, and when its user id attribute is removed. The kill tools end
sessions through the monitor, which removes the entry before invalidating,
and the listing skips and forgets an entry whose session is already gone
instead of failing. The shared SessionUserResolver reads the UserAPI
through the APILocator on every call, so it follows a replaced API in
tests and at runtime.
Unit tests cover the monitor's new behavior and the resolver; integration
tests cover kill-all, single kill and listing with a signed-out session.
Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
Ending one session by token called the monitor without the guard that kill-all has, so a session that had ended on its own between the listing and the kill answered with a server error instead of as a kill. The ids collected for the login token rotation also included the anonymous user when a tracked session ran as that user, which rewrote the shared anonymous record on every kill for no purpose, since that user holds no login token. The single kill now treats an already-ended session as ended, and the anonymous user id is dropped from the rotation set. Two integration tests cover both cases. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
|
On the two items in the automated review (#38469 (comment)), both are intentional and verified:
|
|
On the remaining item of the automated review (#38469 (comment)), the per-request lookup after a cache flush:
|
A session kill ended the session first and then invalidated the login tokens of its users, logging a failure of that second step at WARN. The caller got a success while the user's login cookie still worked, and a retry found no session to kill. Both kill tools now invalidate the tokens first and only then end the session: the single kill lets the failure reach the caller with the session left as it was, kill-all leaves that session out of the count for a later call. The anonymous user id is now excluded ignoring case, as the session monitor compares it. Also from the review: the resolver's constructors are no longer public (the shared instance is the only entry from production code), a failed user lookup is logged at ERROR with the user id and the exception, as the code it replaced did, the save's cache eviction also covers a runtime exception from the permission check, and the Javadoc of the id-only lookup says why it has nothing to sign out. Four integration tests cover the kill order, the anonymous id in another case and the eviction. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
|
…ts now see killSession declares DotDataException since the login tokens are invalidated before the session ends, so the six tests that call it directly have to declare it too. The integration test module did not compile on CI. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
jcastro-dotcms
added
the
PR : dotbot review
|
dotbot code review: Workflow run 43050845052 — attempt 1
openai/gpt-6.1-terra: Object-only sessions escape the new termination mechanism and can expose an unvalidated user after a lookup failure. A concurrent lookup can also leave a deleted user cached after commit. anthropic/claude-haiku-5.5: F1 is retained: the legacy form login stores only the CMS_USER object, so those sessions are never tracked, ended, or status-checked, contrary to the PR's stated guarantee. F2 is rejected because the unvalidated CMS_USER read predates this patch. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · panel · medium Participants: openai/gpt-6.1-terra, anthropic/claude-haiku-5.5 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…saction A copy of the user cached before the commit, as a concurrent lookup would leave behind, is evicted once the delete commits, so a session of that user that the delete did not reach resolves to no user afterwards. The test documents the guarantee the existing cache administrator already gives. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
|
dotbot code review: Workflow run 43055504781 — attempt 1
openai/gpt-6.1-terra: A retained administrator cookie can undo the intended forced sign-out, and WebSocket authentication can bypass the new Login As principal check on lookup failure. anthropic/claude-haiku-5.5: F1 is rejected. Deactivating the impersonated user ends the Login As session and flags it so its remember-me cookie is not reissued. The administrator's remaining cookie authenticates the administrator as themselves, not as the deactivated target, so it gives no escalation. No retained or new actionable defects remain. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · panel · medium Participants: openai/gpt-6.1-terra, anthropic/claude-haiku-5.5 |
The WebSocket configurator asked the session for its user and, when that gave none, loaded a user straight from the session's user id without the status check the session resolution applies. That fallback dates from when the session only handed back its cached user object; the resolution now serves a session that holds only the user id, so the fallback is removed and a session that yields no user for a request yields no user on the socket either. Tests: DotCmsWebSocketConfiguratorTest (new): user id only session, Login As session whose administrator cannot be read, lookup error, no session user. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
|
dotbot code review: Workflow run 43059849476 — attempt 1
openai/gpt-6.1-terra: The retained wrapper issues can restore a rejected impersonated identity or leave Login As state in a subsequent session. anthropic/claude-haiku-5.5: Both ledger issues are retained. VelocityRequestWrapper.removeAttribute refuses USER_ID through NOT_SETTABLE_ATTRIBUTES, so the copied id survives the failed cleanup and a later resolveActive call can restore the impersonated user. VelocitySessionWrapper refuses removal of PRINCIPAL_USER_ID and LoginAsSession.SESSION_KEY, so sign-out through a wrapped session leaves Login As state behind. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · panel · medium Participants: openai/gpt-6.1-terra, anthropic/claude-haiku-5.5 |
|
dotbot code review: Workflow run 43067372717 — attempt 1
openai/gpt-6.1-terra: The patch introduces an invalidated-session race and can partially revoke login tokens when a Login As session kill fails. anthropic/claude-haiku-5.5: F3 stays retained: the legacy form login stores only CMS_USER, so SessionMonitor never tracks those sessions, contrary to the PR's stated guarantee. F1, F2 and F4 are rejected; F4 duplicates F3. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · panel · medium Participants: openai/gpt-6.1-terra, anthropic/claude-haiku-5.5 |
🟡 [P2] LoginFactory.java:274 Legacy form login sessions are never trackedPosted as a general PR comment because the referenced file is not part of this PR's diff. Current code: session.removeAttribute(com.dotmarketing.util.WebKeys.VISITOR);
// session stuff
session.setAttribute(WebKeys.CMS_USER, user);Problem: This login stores only Fix: session.setAttribute(WebKeys.CMS_USER, user);
session.setAttribute(com.liferay.portal.util.WebKeys.USER_ID, user.getUserId()); |
… one unit When a session kill invalidates the login tokens of more than one user (the session user and the user behind a Login As session), the invalidations now run in one transaction: when one of them fails, none is kept, the session is left as it was and the kill can be tried again. The ids are handled in a fixed order, the session user first. The request user resolution returns a user already resolved for the request without reading the session again, as it did before the per-request check. Tests: MaintenanceResourceIntegrationTest (principal invalidation fails, the user's token still works), PortalUtilTest (new, cached user read first). Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
|
On the remapped finding about |
|
dotbot code review: ❌ Review failedThis attempt has no completed verdict. Earlier summaries do not represent this attempt. Head: |
|
dotbot code review: Workflow run 43079965899 — attempt 2
openai/gpt-6.1-terra: Mixed-case login IDs can populate a cache alias that survives deactivation, allowing a session on another node to resolve a stale active user. anthropic/claude-haiku-5.5: F1 is retained at P2 rather than P1. Login passes the raw login id to loadUserById and stores it in USER_ID, and UserCacheImpl.remove does not evict that raw-case alias on deactivation, so the per-request check can read a stale active record. Tracked local sessions are still ended case-insensitively, so the exposure is limited to untracked sessions on other nodes, which the PR documents as node-local. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · panel · medium Participants: openai/gpt-6.1-terra, anthropic/claude-haiku-5.5 |
…eactivation A session may name its user in a letter case other than the stored one. Resolving it fills the user cache through that id, and the cache administrator keys its entries without regard to case, so the eviction made when the user is saved as inactive removes that entry as well. The new integration test keeps that visible: an untracked session holding the id in upper case resolves while the user is active, and resolves to no user once the user is made inactive, with no cache entry left under the session's spelling. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
|
dotbot code review: Workflow run 43254745968 — attempt 1
openai/gpt-6.1-terra: A whitespace-padded login ID can leave a stale active-user cache entry and allow its session to survive deactivation. anthropic/claude-haiku-5.5: F1 is retained: a whitespace-padded login id can keep a stale active user cache entry after deactivation, and the session tools do not match that session to its user. Severity is P2 because reachability of padded ids through the login form was not fully traced. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · panel · medium Participants: openai/gpt-6.1-terra, anthropic/claude-haiku-5.5 |
|
dotbot code review: Workflow run 43267350928 — attempt 1
openai/gpt-6.1-terra: A padded login ID can bypass both the deactivation status check and node-local session termination. anthropic/claude-haiku-5.5: F1 is retained at P2. A padded login id is stored untrimmed in USER_ID and keys the user cache under the raw spelling, while deactivation evicts only the trimmed id and session matching does not trim, so a deactivated user's untracked session can still resolve. Reachability through the login form was not fully traced, which keeps this below P1. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · panel · medium Participants: openai/gpt-6.1-terra, anthropic/claude-haiku-5.5 |
A user id typed with surrounding spaces reached the session as typed when the company authenticates by user id, and a Login As request stored the posted id as typed. The user cache keys its entries without regard to letter case but keeps surrounding spaces, so a session with a padded id filled an entry that the evictions made on a save or a login token change never reached, and the session registry did not match that session to its user. Such a session kept resolving the copy cached before the user was made inactive. The login and Login As now store the id of the loaded user record, the user factory trims the id before reading or filling the cache, and the session registry ignores surrounding spaces when matching a session to a user. Co-Authored-By: Claude Mythos 5.1 <noreply@anthropic.com>
|
dotbot code review: Workflow run 43287571346 — attempt 1
openai/gpt-6.1-terra: GraphQL can return a cached response associated with a session identity that failed the new status or principal check. anthropic/claude-haiku-5.5: Both ledger issues depend on GraphqlCacheKey reading the retained object-only USER copy after a lookup failure. An anonymous request computes a key without the sessionUser component, so it cannot read an entry keyed to that user, and no anonymous read of private data was found. A response computed while the session user was refused could be stored under that user's key, which is a cache-correctness concern rather than a leak. Confidence is moderate because runtime tests were not run and the other PR-thread items were not re-verified. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · panel · medium Participants: openai/gpt-6.1-terra, anthropic/claude-haiku-5.5 |
| + "', treating the session as having no user for this request: " | ||
| + e.getMessage(), e); | ||
| // the copy stays when it is the only thing that identifies the session | ||
| if (null != session.getAttribute(WebKeys.USER_ID)) { |
There was a problem hiding this comment.
🔴 [P1] SessionUserResolver.java:130 Fail closed on lookup errors
Current code:
if (null != session.getAttribute(WebKeys.USER_ID)) {
session.removeAttribute(WebKeys.USER);
}
return Optional.empty();Problem: GraphQL can serve a cached private response using the retained session ID after authentication falls back to anonymous.
Fix:
throw new DotRuntimeException("Cannot validate session user", e);Also fail the request when the Login As principal lookup returns UNKNOWN. Alternatively, key GraphQL responses by the authenticated user rather than session attributes.
Summary
The user a session belongs to is now resolved through a new
SessionUserResolverthat reads the current user record (cache-backed) on every request, instead of theUsercopy stored in the session at login. A user that is no longer active, or no longer exists, resolves to no user and the session is signed out. Making a user inactive, or deleting the user, also ends that user's tracked sessions on the node and rotates the user's login token, and the Maintenance session tools rotate the login tokens of the users whose sessions they end.Changes
SessionUserResolver(new,com.dotcms.cms.login): resolves the session user fromUSER_ID(or the storedUsercopy), re-checks the record throughUserAPI.loadUserById, signs the session out (user id, cached copy, Login As attributes) when the user is missing or inactive, refreshes the cached copy when the record changed, and checks the administrator behind a Login As session as well. On a lookup error the cached copy is dropped for that request and the user id stays, so the next request checks again.ALLOW_INACTIVE_ACCOUNTS_TO_LOGINis honoured.PortalUtil.getUser(HttpSession)delegates to the resolver;getUser(HttpServletRequest)resolves a request-levelUSER_IDthrough it as well.CategoriesWebAPIusesPortalUtil.getUser(request)instead of its own session read.UserAPIImpl.save: when a stored active user is saved as inactive, the login token is rotated (invalidateLoginTokens, the new token copied to the saved object) and the user's tracked sessions are ended after commit.deleteends the tracked sessions.UserFactory.loadStoredUserreads the stored record without the cache, because the object being saved can be the cached instance. The permission and validation errors ofsavenow drop the possibly modified cached instance.SessionMonitor: publictrack(HttpSession)andendSessionsOf(userId)(ends every tracked session of that user, including Login As sessions where the user is the principal, with the remember-me flag set so the cookie is not reissued; a session that fails to end is forgotten and the rest still end). Sessions are tracked fromattributeAdded/attributeReplacedonUSER_ID, so every login path registers the session before the next request.MaintenanceResource: kill-session and kill-all-sessions rotate the login tokens of the session's user and, for a Login As session, of the principal. The listing shows a session whose user can no longer be resolved under the stored user record instead of the anonymous user.openapi.yamlregenerated for the updated descriptions.Tests
dotcms-core, 40 tests):SessionUserResolverTest(new, 24),SessionMonitorTest(15),UserAPIImplTest(new, 1).UserWebAPIImplTest(user made inactive or deleted after login is no longer the logged-in user),WebResourceIntegrationTest(back-end session answered 401 after the user is made inactive or deleted),LoginTokenInvalidationTest(token rotated and tracked session ended on deactivation and deletion; unchanged on an ordinary save and on reactivation; a realdoActionLoginregisters the session),MaintenanceResourceIntegrationTest(kill-session and kill-all reject the affected users' tokens and keep the caller's),UserResourceIntegrationTest(Login As keeps working with an active target; the session is signed out when the impersonated user or the administrator is made inactive). All green locally.Risk
SessionMonitormap is per JVM); the status re-check applies on every node. Ending sessions across nodes is a follow-up.Checklist
Fixes dotCMS/private-issues#902
🤖 Generated with Claude Code