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

Jump to bottom

ERROR dispatch can cause two sessions to be created #229

Closed
tsachev opened this issue Jun 25, 2015 · 5 comments
Closed

ERROR dispatch can cause two sessions to be created #229

tsachev opened this issue Jun 25, 2015 · 5 comments
Assignees
@rwinch
Labels
type: bug A general bug
Milestone
1.0.2

Comments

@tsachev
Copy link
Contributor

tsachev commented Jun 25, 2015

Updated Description

After further feedback from @tsachev the issue is reproduced:

  • Initially there is no session created on the server and no session cookies in the browser.
  • The controller/servlet creates a new session and sets an attribute. then throw an exception
  • The error is handled and the SessionRepositoryFilter is invoked on the ERROR dispatch

This happens because the wrapped request that is caching the current session is not there anymore. It is a new HttpServletRequest object that is no longer wrapped. Instead, we should save the HttpSessionWrapper currentSession in a HttpServletRequest attribute.

Original Description

If a request creates a new session and then forwards (requestDispatcher.forward()) to another servlet/view which tries to update the session - two session are created.

if SessionRepositoryFilter is registered for DisptcherType.FORWARD - two spring sessions are created.
if SessionRepositoryFilter is not registered for DisptcherType.FORWARD - one spring session is created and one tomcat session.

I can see that two Set-Cooke headers are sent to the browser.

HTTP/1.1 500 Internal Server Error
Server: Apache-Coyote/1.1
Set-Cookie: SESSION=8aa5f36e-820c-43bf-ba68-3797fd70b20f; Path=/storefront/; Secure; HttpOnly
Set-Cookie: SESSION=acc2a3df-ab9a-4080-afa3-c740fee75f5b; Path=/storefront/; Secure; HttpOnly
Content-Type: text/html;charset=UTF-8
Content-Language: en-US
Content-Length: 0
Date: Thu, 25 Jun 2015 14:56:10 GMT
Connection: close
@rwinch rwinch added this to the 1.0.2 milestone Jun 25, 2015
@rwinch
Copy link
Member

rwinch commented Jul 26, 2015

@tsachev Thanks for the report. I have modified the httpsession sample in a branch (rwinch/spring-session/tree/gh-229) to try to reproduce the issue and am unable to do so. The steps I attempted:

  • The SessionRepositoryFilter is configured for REQUEST, ERROR, and ASYNC (it is not configured for FORWARD as this should generally not be necessary)
  • Ensure Redis is running locally on the default port
  • Start the application using ./gradlew :samples:httpsession:tomcatRun
  • Submit the form with a value for the attribute name and attribute value
  • The request sends a post to "/forward" which is handled by the ForwardServlet by:
    • Setting a session attribute named forward
    • forwarding to the SessionServlet which updates the attribute provided in the form

When looking at my cookies, I only see a single SESSION cookie. When looking in Redis I only see a single session. Both the attribute set before the forward and after the forward are displayed.

I'm wondering if the issue is related to the fact that your response is an ERROR? You may need to ensure the SessionRepositoryFilter is mapped to ERROR dispatch types as well.

Can you try and provide more details on how to reproduce this issue? Perhaps a sample?

Thanks!
Rob

@rwinch rwinch self-assigned this Jul 26, 2015
@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label Jul 26, 2015
@rwinch
Copy link
Member

rwinch commented Jul 26, 2015

@tsachev PS Perhaps you could also let me know which version of Spring Session you are using. If you are not using 1.0.1.RELEASE perhaps you can try updating and seeing if that fixes your issue.

@tsachev
Copy link
Contributor Author

tsachev commented Jul 27, 2015

I will try to make up a simple project that reproduces the problem.

But generally I think the flow goes like this.

  • Initially there is no session created on the server and no session cookies in the browser.
  • The controller/servlet creates a new session and sets an attribute. then throw an exception (thus the error in the response above).
  • A custom exception handler forwards request to an error page (a jsp without <%@ page session="false" %>)

@rwinch
Copy link
Member

rwinch commented Jul 27, 2015

@tsachev Thanks for the reply. This was enough information for me to reproduce the problem. I will get a fix for this issue in 1.0.2.

@rwinch rwinch added type: bug A general bug and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 27, 2015
@rwinch rwinch mentioned this issue Jul 27, 2015
Closed
@rwinch rwinch changed the title when using forward two session may be created for the same request. ERROR dispatch can cause two sessions to be created Jul 27, 2015
rwinch pushed a commit that referenced this issue Jul 27, 2015
Fix Tests
8929e85 
These tests passed, but were technically incorrect. The
invalid tests were noticed when fixing gh-229.

Issue: gh-229
rwinch pushed a commit that referenced this issue Jul 27, 2015
currentSession saved on HttpServletRequest attribute
3d93f2c 
Previously, if the following happened:

* New Session Created
* Exception thrown
* Exception processed by error handler within Servlet
* Error Handler used a session

The result would be two sessions were created. This means the
data from the first session was also lost. This happend
because ERROR dispatch is a separate Filter invocation where
the request is no longer wrapped.

This commit ensures that currentSession is saved on a
HttpServletRequest attribute so that the ERROR dispatch sees
that a session was already created.

Fixes: gh-229
@rwinch
Copy link
Member

rwinch commented Jul 27, 2015

Thanks for the report! I have fixed this in master and it will be available in the 1.0.2 release.

While I did test this myself, I'd appreciate you trying out the latest SNAPSHOT and ensuring that it resolved your issue. You can obtain the spring-session-1.0.2.BUILD-SNAPSHOT.jar from our maven repository:

<repository>
    <id>spring-snapshot</id>
    <url>https://repo.spring.io/libs-snapshot</url>
</repository>

PS: Based on your report I found another bug which we will also be fixing. See #251 Thanks again!

@rwinch rwinch closed this as completed Jul 27, 2015
rwinch pushed a commit that referenced this issue Jul 28, 2015
CookieHttpSessionStrategy only writes same Session once
d5484e1 
If SessionRepositoryRequestWrapper.commitSession() is invoked twice
when a new session is created, then CookieHttpSessionStrategy will
add the same cookie twice. A couple examples of how this could happen:

* The response is committed and
  SessionRepositoryResponseWrapper.onResponseCommitted() invokes
  SessionRepositoryRequestWrapper.commitSession(). Then the finally
  block in SessionRepositoryFilter invokes
  SessionRepositoryRequestWrapper.commitSession() again.
* The new session is initialized and an Exception is thrown (i.e.
  gh-229). The SessionRepositoryFilter invokes
  SessionRepositoryRequestWrapper.commitSession() in the REQUEST
  dispatch. Then in the ERROR dispatch SessionRepositoryFilter invokes
  SessionRepositoryRequestWrapper.commitSession() invokes it again.

This commit ensures if the same Session is passed into
CookieHttpSessionStrategy multiple times within the same HttpServletRequest
it is only written once by keeping track of the sessions on a request
attribute.

Fixes gh-251
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rwinch rwinch

Labels
type: bug A general bug
Projects
None yet
Milestone
1.0.2
Development

No branches or pull requests
2 participants
@tsachev @rwinch