dbeaver/cloudbeaver#4249 added session rotation#4274
dbeaver/cloudbeaver#4249 added session rotation#4274
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Nexus6v2
force-pushed
the
dbeaver/cloudbeaver#4249-session-id-rotation
branch
from
668341b to
0ae568d
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements session rotation during authentication to mitigate session fixation attacks (per #4249) by introducing a rotateSession helper and wiring HttpServletRequest into the auth login flows.
Changes:
- Added
CBSessionManager.rotateSession(...)and event-handler migration support inBaseWebSession. - Updated auth service API/binding to pass
HttpServletRequestintoauthLogin, and rotated the session at the start ofauthLogin/federatedLogin. - Minor refactors/copyright year updates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/bundles/io.cloudbeaver.service.auth/src/io/cloudbeaver/service/auth/impl/WebServiceAuthImpl.java | Rotates session during authLogin and federatedLogin attempts. |
| server/bundles/io.cloudbeaver.service.auth/src/io/cloudbeaver/service/auth/WebServiceBindingAuth.java | Passes servlet request into authLogin via GraphQL wiring. |
| server/bundles/io.cloudbeaver.service.auth/src/io/cloudbeaver/service/auth/DBWServiceAuth.java | Extends authLogin signature to include HttpServletRequest. |
| server/bundles/io.cloudbeaver.server.ce/src/io/cloudbeaver/service/session/CBSessionManager.java | Adds rotateSession implementation and small findWebSession refactor. |
| server/bundles/io.cloudbeaver.model/src/io/cloudbeaver/model/session/BaseWebSession.java | Adds event handler migration method used during rotation. |
| server/bundles/io.cloudbeaver.service.auth/src/io/cloudbeaver/service/auth/handler/WSAuthSessionEventHandler.java | Copyright year update. |
| server/bundles/io.cloudbeaver.service.auth/src/io/cloudbeaver/service/auth/WebAsyncAuthJob.java | Copyright year update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| boolean forceSessionsLogout | ||
| ) throws DBWebException { | ||
| try { | ||
| // Rotate web session during each login attempt to prevent session fixation attacks |
There was a problem hiding this comment.
What if this is an additional login (e.g. AWS)?
The same for federatedLogin.
There was a problem hiding this comment.
Good question.
My guess is session should also be rotated for additional logins, since we would have the same vulnerability:
- User logs in
- Session cookie is compromised
- User does additional login
- Compromised session cookie gains access to new auth
Rotated session should also keep the existing auth
I will update the PR
There was a problem hiding this comment.
Decided on the call to keep rotation only for the first login (anonymous only)
| } | ||
| } | ||
|
|
||
| public void migrateEventHandlersTo(@NotNull BaseWebSession target) { |
There was a problem hiding this comment.
How will it work in case of multi-node env?
And why do we need to migrate event handlers at all? Generally new login means that old session is no more valuable, we should just close it
There was a problem hiding this comment.
How will it work in case of multi-node env?
I believe it will only be different in case if sticky-sessions routing based on this cookie is used. As far as I know it's not, so it should behave the same.
And why do we need to migrate event handlers at all? Generally new login means that old session is no more valuable, we should just close it
There is an open web socket that we need to migrate to new session, otherwise the flow breaks and UI waits for auth with an endless spinner
Closes #4249