Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Highly confident there is a casting / deserialisation bug in SpringSessionBackedReactiveSessionRegistry #3182

Open
dreamstar-enterprises opened this issue Aug 29, 2024 · 3 comments
Assignees
Labels
status: feedback-provided Feedback has been provided type: bug A general bug

Comments

@dreamstar-enterprises
Copy link

dreamstar-enterprises commented Aug 29, 2024

Describe the bug

When my logout handler calls this

fun invalidateSession(sessionId: String): Mono<Void> {
        logger.info("Invalidating sessionId: ${sessionId}")
        // handle the session invalidation process
        return reactiveSessionRegistry.getSessionInformation(sessionId)
            .flatMap { session ->
                // invalidate session
                session.invalidate()
                    .then(
                        // delete session
                        webSessionStore.removeSession(sessionId)
                    )
                    .doOnSuccess {
                        logger.info("Session invalidated and removed: ${sessionId}")
                    }
                    .doOnError { error ->
                        logger.error("Error invalidating session: ${sessionId}", error)
                    }
            }
    }

The following function inside SpringSessionBackedReactiveSessionRegistry gets called:

	@Override
	public Mono<ReactiveSessionInformation> getSessionInformation(String sessionId) {
		return this.sessionRepository.findById(sessionId).map(SpringSessionBackedReactiveSessionInformation::new);
	}

The inner class is implemented as follows:

class SpringSessionBackedReactiveSessionInformation extends ReactiveSessionInformation {

		SpringSessionBackedReactiveSessionInformation(S session) {
			super(resolvePrincipalName(session), session.getId(), session.getLastAccessedTime());
		}

		private static String resolvePrincipalName(Session session) {
			String principalName = session
				.getAttribute(ReactiveFindByIndexNameSessionRepository.PRINCIPAL_NAME_INDEX_NAME);
			if (principalName != null) {
				return principalName;
			}
			SecurityContext securityContext = session.getAttribute(SPRING_SECURITY_CONTEXT);
			if (securityContext != null && securityContext.getAuthentication() != null) {
				return securityContext.getAuthentication().getName();
			}
			return "";
		}

		@Override
		public Mono<Void> invalidate() {
			return super.invalidate()
				.then(Mono.defer(() -> SpringSessionBackedReactiveSessionRegistry.this.sessionRepository
					.deleteById(getSessionId())));
		}

	}

But, here on this line:

SecurityContext securityContext = session.getAttribute(SPRING_SECURITY_CONTEXT);

SPRING_SECURITY_CONTEXT is received from Redis as a HashMap or LinkedHashMap, so cannot be cast to SecurityContext (w/o proper de-serialisation)

This is EXACTLY the error I see:

Screenshot 2024-08-30 at 00 01 35

Two calls to get session?

Also, I'm not sure if this is calling Redis again to get the security context, but is it necessary?, given just before calling /logout endpoint, the session / security context is retrieved anyway, (see below.)

SessionId would come from the session, here, when this is called at the very start fun invalidateSession(sessionId: String): Mono ) so calling getSessionInformation(String sessionId) and with it, this.sessionRepository.findById(sessionId), again, seems a bit wasteful...?

Screenshot 2024-08-30 at 00 02 24

To Reproduce
See above, just try the above, with sessions stored to redis, then try to invalidate a session calling the above functions

Expected behavior
The casting should be properly deserialised. A linkedHashmap cannot be cast to a SecurityContext object directly

Sample

See above. Github code can be found here:

My implementations
https://github.com/dreamstar-enterprises/docs/blob/master/Spring%20BFF/BFF/src/main/kotlin/com/frontiers/bff/auth/sessions/SessionControl.kt
https://github.com/dreamstar-enterprises/docs/blob/master/Spring%20BFF/BFF/src/main/kotlin/com/frontiers/bff/auth/sessions/SessionRegistryConfig.kt

Spring implementation (where error is I believe)
https://github.com/spring-projects/spring-session/blob/main/spring-session-core/src/main/java/org/springframework/session/security/SpringSessionBackedReactiveSessionRegistry.java

@dreamstar-enterprises dreamstar-enterprises added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Aug 29, 2024
@dreamstar-enterprises
Copy link
Author

Screenshot 2024-08-31 at 21 01 32

@marcusdacoregio
Copy link
Contributor

Hi @dreamstar-enterprises, thanks for the report.

Your links are returning a 404.

I tried it with a sample from the repository and it worked fine. I might be missing something.
Can you please provide a minimal, reproducible sample with only Spring Session Data Redis?

@marcusdacoregio marcusdacoregio added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 4, 2024
@marcusdacoregio marcusdacoregio self-assigned this Sep 4, 2024
@dreamstar-enterprises
Copy link
Author

Hi Marcus,

I fixed the links

The root cause of the problem is this

image

When i use session.getAttributes, objects are returned from Redis as HashedMaps

So this line fails in your implementation does not work. I've tested it several times:

https://github.com/spring-projects/spring-session/blob/main/spring-session-core/src/main/java/org/springframework/session/security/SpringSessionBackedReactiveSessionRegistry.java#L119

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants