jakartaee / servlet

Jakarta Servlet
https://eclipse.org/ee4j/servlet
Other
261 stars 85 forks source link

Fix #546 - mechanism to access HttpSession outside of request #547

Closed markt-asf closed 8 months ago

joakime commented 1 year ago

This seems like a partial solution.

The HttpSession accessed from this new accessor can do what? Is it read-only? (I hope so, otherwise syncing changes into/out of the HttpSession when held long term in a place like websocket would be a pain in the ass)

markt-asf commented 1 year ago

It provides full read/write access to the session.

The use of the Consumer interface (see the dev list thread for the details) is so WebSocket isn't holding a reference to the actual session.

gregw commented 1 year ago

One way to think of this is that the inner consumer takes the place of a Servlet accessing a session. The outer consumer takes the place of a session manager that scopes the call to the "Servlet" by ensuring the session is activated and updating access time/count etc and then counts decremented after the call

joakime commented 1 year ago

Has anyone bounced this idea off the CDI folks? Feels like they will have to change their servlet integration implementations to support this.

markt-asf commented 12 months ago

Why should changing the session ID always prevent the Accessor from continuing to work? I can see some circumstances where that could happen but there are some cases where the Accessor could continue to work.

I also don't see the security concern if an application uses the same session object (with different IDs) before and after authentication. Any nefarious client activity (such as session fixation attacks) can only reference the session by ID and the change in ID is sufficient to protect against that sort of behaviour.

gregw commented 12 months ago

@markt-asf you're right that it could sometimes be implemented to persist over an ID change, but it cannot be done so generally. Specifically in a cluster if the session is locally passivated, and the ID is changed on another node, then the semantics are really poorly defined.

Thus I was going for well defined semantics that are easy to implement. The accessor keeps the session ID locally and if it can find a valid session when it is accessed, then all is good.

If we allow an accessor to remain valid over an ID change, then we either are going to have inconsistent behaviour depending on the actual session implementation or deployment, OR we will need to implement some complex mechanism to update all accessors in a cluster.

I do think it is of slight concern that a websocket connection established before a session is authenticated may be able to maintain access to the session after authentication. I don't see any huge vulnerability immediately, but the whole point of changing a session's ID at authentication is that the session then becomes accessible only to clients that are given that ID after authentication. Put another way, when code calls a method to change the session id, it is doing so to reduce the number of references to that session to zero, and it can then hand out new references as it sees fit. Having accessors bypass that mechanism feels like a violation of an important contract. Note that this is not about what happens if a Consumer<HttpSession> is actively being called when a session ID changes, as that is equivalent to what happens if there are concurrent request. Calling the accessor is semantically equivalent to a new request arriving and unless the client of that new request has explicitly been given the new session ID, then it cannot access the session.

janbartel commented 12 months ago

FWIW I've implemented the HttpSession.Accessor interface in an experimental jetty 12 branch and it was easy to do. I'll add some more test cases to be sure, but so far it looks very promising.

markt-asf commented 8 months ago

Implementing this in Tomcat was also simple. PR LGTM. I think we can merge this. @gregw if you can approve the changes you requested that should enable this to me merged with having to dismiss your review.

markt-asf commented 8 months ago

Forced push was just to rebase to fix merge conflicts in spec doc