Skip to content

fix: getSession(false) returns null after session invalidation (#115) - #116

Merged
mcollovati merged 2 commits into
mainfrom
logout-tests
Jun 30, 2026
Merged

fix: getSession(false) returns null after session invalidation (#115)#116
mcollovati merged 2 commits into
mainfrom
logout-tests

Conversation

@Artur-

@Artur- Artur- commented Jun 25, 2026

Copy link
Copy Markdown
Member

MockRequest.getSession(false) returned the stale, invalidated session instead of null once the session had been invalidated, violating the servlet container contract. This broke logout: Spring Security's SecurityContextLogoutHandler invalidates the HttpSession and then calls HttpSessionSecurityContextRepository.saveContext, which looks up request.getSession(false). In a real container that returns null and the empty-context save is a no-op, but the mock handed back the invalidated MockHttpSession, so removing the security-context attribute threw IllegalStateException("invalidated").

Honour the servlet contract: once invalidated, getSession(false) returns null and getSession(true) creates a fresh session. Add LogoutTest covering the issue #115 sequence plus the logout->redirect-to-login and logout->new-login flows, and update MockRequestTest to assert the corrected behaviour.

Fixes #115

MockRequest.getSession(false) returned the stale, invalidated session
instead of null once the session had been invalidated, violating the
servlet container contract. This broke logout: Spring Security's
SecurityContextLogoutHandler invalidates the HttpSession and then calls
HttpSessionSecurityContextRepository.saveContext, which looks up
request.getSession(false). In a real container that returns null and the
empty-context save is a no-op, but the mock handed back the invalidated
MockHttpSession, so removing the security-context attribute threw
IllegalStateException("invalidated").

Honour the servlet contract: once invalidated, getSession(false) returns
null and getSession(true) creates a fresh session. Add LogoutTest
covering the issue #115 sequence plus the logout->redirect-to-login and
logout->new-login flows, and update MockRequestTest to assert the
corrected behaviour.
@Artur-
Artur- requested a review from mcollovati June 25, 2026 17:53
@Artur-

Artur- commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

/format

@github-actions

This comment has been minimized.

Comment on lines +96 to +98
Assertions.assertThrows(IllegalArgumentException.class,
() -> window.navigate(ProtectedView.class));
Assertions.assertInstanceOf(LoginView.class, window.getCurrentView());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is weird behavior here: navigating on an existing window redirects to Login view, but if I create a new window, the protected page is accessible.

This might be acceptable, since user represents a logged-in user, but it is not intuitive. If we agree on keep this behavior, we should explicitly document it.
Or should the BrowserlessUserContext be closed on session invalidation?
Maybe this topic deserves a separate ticket.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part do you find unintuitive?

@mcollovati mcollovati Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BrowserlessUserContext looks paired to a user session. It starts with a logged-in user, and all windows I create can navigate to protected views.
If I simulate a press to a logout button, the current window is then unable to navigate to protected pages and navigation is redirected to the login view.
But if I create a new window, that one is authenticated again, but not the old one.
So I have a user session that is half authenticated and half unauthenticated.
This looks confusing to me.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so the problem is that it should really invalidate the "user" object also so that all windows, both existing and new, are unauthenticated. That's how a real application would work if you log out in one tab.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I would expect. If you need to log in again, you should create a new context (e.g. app.newUser("john", "USER")).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create a separated ticket to fix this issue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollovati
mcollovati merged commit 07b4a4f into main Jun 30, 2026
6 checks passed
@mcollovati
mcollovati deleted the logout-tests branch June 30, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Invalid session" exception when clicking the logout button of the app during Browserless testing

3 participants