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

WebSession/WebSessionStore API are silent on saving a session that may have been invalidated [SPR-17051] #21589

Closed
spring-projects-issues opened this issue Jul 17, 2018 · 7 comments
Assignees
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jul 17, 2018

Yuriy Sazonets opened SPR-17051 and commented

DefaultWebSessionManager#save checks whether previously retrieved session is active (started) and not expired, but doesn't check if it actually exists in the session store.

Consider scenario:

  1. Long-running WebSocket request A.
  2. Logout during request A => session id gets changed.
  3. Request A ends and gets committed.
  4. DefaultWebSessionManager#save gets invoked with old session id (which is not valid anymore) and proceeds with saving.
  5. ReactiveRedisOperationsSessionRepository#save doesn't validate if the session passed in exists in the store, and just applies the delta (which in this case is only lastAccessedTime), which results in an broken session entry with only lastAccessedTime attribute.
  6. Subsequent requests to old session id fail because session data is inconsistent in the repository.

This causes major problems in production code. More details (and sample project to reproduce the issue) here: spring-projects/spring-session#1111


Affects: 5.0.7

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 17, 2018

Rossen Stoyanchev commented

Several items:

  1. It surprising to me that the handshake request does not complete until the end of the session. Violeta Georgieva is this expected behavior? Here we call sendWebSocket and according to the Javadoc that returns "a Mono completing when upgrade is confirmed". However it doesn't seem to return until the end of the session. FWIW on the other supported servers, we complete the HTTP request at the time of upgrade, while the session completes independently.

  2. Assuming a long running request, the isExpired check was always there for a potential long request but since invalidate() was added a bit later, it is also possible for the session to have become invalid.

Vedran Pavic I am wondering how to best address this? We could add a "Mono<Boolean> checkValid()" on WebSession, but we'd also have to document on save() that the caller is responsible to check if the session is still valid which would result in required boilerplate code session.checkValid().then(session.save()). Alternatively we could update the Javadoc on save() that the implementation should make sure first the session is still not expired and valid.

  1. Looking at the InMemoryWebSession implementation, we need to change the save method to explicitly check for expired/invalid state and raise an error.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 17, 2018

Vedran Pavic commented

Thanks for the ping Rossen Stoyanchev - between the two I think making the contract of save() explicit to ensure implementations validate the state of the session before persisting the changes looks to be both simpler and safer option.

I'd be also interested to hear from Rob Winch on this topic.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 18, 2018

Rossen Stoyanchev commented

Modified title, originally:
"DefaultWebSessionManager#save doesn't check if the session actually exists in the session store"

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 18, 2018

Rossen Stoyanchev commented

One extra thought... wouldn't the problem still exist even with a check before save(), since the session could become invalid in the short period after the check and before the save? So perhaps this is an argument to leave it to the save()? Would the Redis implementation be able to provide better guarantees that a save would not override an invalidate?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 18, 2018

Vedran Pavic commented

Yes, I agree - that's why I referred to the option of making it responsibility of save() operation as safer option in my previous comment.

Besides that, we've been recently hit with a few other reports involving race condition scenarios, where Spring Session's SessionRepository implementations are causing issues due to insufficient validation of the session's state in the underlying data store before executing update operation.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 19, 2018

Rossen Stoyanchev commented

I've updated WebSession. Hopefully that makes sense but if not let me know.

I've also created Reactor Netty #393 to keep track of why the handshake request does not complete until the end of the WebSocket session.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 19, 2018

Vedran Pavic commented

Looks good Rossen Stoyanchev. I've just opened gh-1888 to apply some minor polish.

@spring-projects-issues spring-projects-issues added type: enhancement in: web labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 5.1 RC1 milestone Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants