Reusing a deleted session.

[nkavian]

Version

• vert.x web: 3.2.1

  Context

It's standard practice during a user login to first invalidate the session and then populate session variables. Implementations in other platforms like PHP, Tomcat, SparkJava, etc, will reset your cookie as part of that invalidation. When a developer calls session.destroy() he will not know the internal implementation and assume that it is functionally equivalent to other platforms.

A) When getData() is called and it generates a new map, you need to set deleted=false; The current behavior is that I call session.destroy() and then session.put(); to repopulate the session. However when the page gets rendered destroyed is true and the session is thrown away.. https://github.com/vert-x3/vertx-web/blob/master/vertx-web/src/main/java/io/vertx/ext/web/sstore/impl/SessionImpl.java#L151

B) When getData() is called and it generates a new map, you need to also generate a new GUID and place it into the session cookie.

Both of these are security best practices. You can probably find full details in OWASP.

[pmlopes]

That feels awkward, first you delete then you use, if we were talking C this would be a segmentation fault! :) Why would you do that it feels unnatural.

Probably a clear() method could be added to clear any existing data.

[nkavian]

You might be thinking too much at the implementation level when you refer to seg fault. Think of the higher level use case. A developer needs to (re)generate a new session one or more times before context.request().end() is called. Solving that statement correctly will resolve my earlier (A) and (B) above.

Please read this from OWASP for a deeper understanding why this is necessary for security purposes. https://www.owasp.org/index.php/Session_Management_Cheat_Sheet#Renew_the_Session_ID_After_Any_Privilege_Level_Change

Another way to restate the problem: Vertx Web can't be used in PCI compliant environments out of the box because of the above issue.

Node is pretty popular where I work, but as we all know it suffers from the single thread issue. I'd love to see Node replaced by Vertx someday.

At the very least, getData() should have been consistent. If you create a new map it should set deleted=false, or alternatively keep deleted=true and throw an exception that the session is invalid.. Let me know which way you decide. I'm eager to see a fix either from Vertx or I'll just create a local implementation. Thanks.

[pmlopes]

If you are eager to see if fixed and will make a local implementation why not be part of the community and propose a proper fix as a pull request? That would be a win win situation I think

[nkavian]

I'll work on it. Is there any documented setup guide? I'm having an issue with Eclipse complaining about a build error and am not sure if there is some setup I need to perform.

Errors occurred during the build.
Errors running builder 'Java Builder' on project 'vertx-web'.
java.lang.NullPointerException

[pmlopes]

Personally I'm don't use Eclipse but I'll check it and see if i get the same problem

[pmlopes]

I've just installed Eclipse Neon for java developers and imported existing maven project. This prompted me to install m2e plugin which i accepted and i can now Run as mvn install and it works. I notice that it uses my maven settings and this could be what you're missing, since the development version depends on snapshot builds which are not on maven central but on oss.sonatype.org you should have something like this:

<settings>

<profiles>
  <profile>
     <activation><activeByDefault>true</activeByDefault></activation>
     <repositories>
       <repository>
         <id>snapshots-repo</id>
         <url>https://oss.sonatype.org/content/repositories/snapshots</url>
         <releases><enabled>false</enabled></releases>
         <snapshots><enabled>true</enabled></snapshots>
       </repository>
     </repositories>
   </profile>
</profiles>

</settings>

[pmlopes]

For reference you might have a look at yoke's session code:

https://github.com/pmlopes/yoke/blob/develop/framework/src/main/java/com/jetdrone/vertx/yoke/middleware/YokeRequest.java

Vertx-Web was influenced by that project but was not a 1-1 copy.

[nkavian]

I've made the changes but I'm having a hard time fixing a few JUnit tests. The level of asynchronicity is driving me nuts. I have LocalSessionHandlerTests passing but ClusteredSessionHandlerTest has several tests that fail.

[pmlopes]

can you share your code, maybe i can assist there

[nkavian]

The code is here: https://github.com/nkavian/vertx-web/tree/issues/issue-399 Here is the specific place I think is causing an issue: https://github.com/nkavian/vertx-web/blob/issues/issue-399/vertx-web/src/main/java/io/vertx/ext/web/handler/impl/SessionHandlerImpl.java#L115

When using the LocalSessionStore, line 117 executes right after line 116. When using the ClusteredSessionStore, line 116 executes, then the route processes and hits a NPE on null session, and then a few seconds later line 117 executes to actually populate the session. Using a latch to synchronize the calls causes a blocked thread.

A related note. Lines 193-200 - Hazelcast stores values deterministically to the same node. There is no propagation delay. All the retry logic does is waste 5 full seconds trying to read a nonexistent value every 5 ms. Now imagine that the Hazelcast cluster is being rebooted and/or developer flushes the map. All clients will hang for 5 seconds while the cluster is pounded every 5ms per client. More specifically, a developer shuts down his JVM to rebuild his code, but the browser is referring to a session ID that no longer exists in Hazelcast because Hazelcast was also in the same JVM. If the developer creates his own session store, let them worry about how to deterministically find the session and perform retries instead of assuming we should retry.