Wicket
  1. Wicket
  2. WICKET-476

Backport RequestCycle#updateSession and pagemap#session removal to 1.2.x

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.5
    • Fix Version/s: 1.2.6
    • Component/s: None
    • Labels:
      None

      Description

      A cleanup bug in the requestcycle may leave dirty objects in the dirtyObjects threadlocal. Removing RequestCycle.updateSession and pagemap#session as we did in 1.3 solves this particular problem.

        Activity

        Hide
        Martijn Dashorst added a comment -

        To clarify the issue a bit more: this issue solves an infrequent occurring security bug, best
        described by the following comment:

        > We have the weirdest thing going on, which is as random and rare
        > as it is deadly. Every once in a while, on a blue moon, the user can see
        > other person's data on his screen! A simple click on the tree - and he got
        > somebody else's data. Then, after more clicks, they disappear.

        This occurs really infrequently, but is serious and deadly enough to warrant a new release (1.2.6).

        We deduced the root of this problem to be a ThreadLocal (Session#dirtyObjects) that was not
        cleaned up at the end of a request, leaving stray data in the thread for another session to pick
        up. The ThreadLocal was not cleaned up in very specific circumstances and the session cross
        over only happened in rare circumstances.

        This issue has been resolved by always cleaning up the thread local and never occurred on our
        servers since.

        Show
        Martijn Dashorst added a comment - To clarify the issue a bit more: this issue solves an infrequent occurring security bug, best described by the following comment: > We have the weirdest thing going on, which is as random and rare > as it is deadly. Every once in a while, on a blue moon, the user can see > other person's data on his screen! A simple click on the tree - and he got > somebody else's data. Then, after more clicks, they disappear. This occurs really infrequently, but is serious and deadly enough to warrant a new release (1.2.6). We deduced the root of this problem to be a ThreadLocal (Session#dirtyObjects) that was not cleaned up at the end of a request, leaving stray data in the thread for another session to pick up. The ThreadLocal was not cleaned up in very specific circumstances and the session cross over only happened in rare circumstances. This issue has been resolved by always cleaning up the thread local and never occurred on our servers since.
        Hide
        Johan Compagner added a comment -

        i think that is a bit dangerous. IF (and then it is bug that getSession is there purely to see if the bug is still there somehow...) but if getSession() is still wrong
        and we throw there an exception but that pagemap is somehow already in the http session then we have a problem. Because that session is then completely not useable anymore (the exception will happen everytime)
        So the only real other option is session.invalidate()...

        Show
        Johan Compagner added a comment - i think that is a bit dangerous. IF (and then it is bug that getSession is there purely to see if the bug is still there somehow...) but if getSession() is still wrong and we throw there an exception but that pagemap is somehow already in the http session then we have a problem. Because that session is then completely not useable anymore (the exception will happen everytime) So the only real other option is session.invalidate()...
        Hide
        Jean-Baptiste Quenot added a comment -

        Shouldn't we throw an exception in PageMap.getSession() instead of logging an error to make it obvious to the end user that there is something wrong? Sometimes people don't pay much attention to the logs.

        Show
        Jean-Baptiste Quenot added a comment - Shouldn't we throw an exception in PageMap.getSession() instead of logging an error to make it obvious to the end user that there is something wrong? Sometimes people don't pay much attention to the logs.
        Hide
        Jean-Baptiste Quenot added a comment -

        For reference this is the associated Subversion commit:

        r529917 | jcompagner | 2007-04-18 10:42:01 +0200 (Mer, 18 avr 2007) | 1 line
        Changed paths:
           M /incubator/wicket/branches/wicket-1.2.x/wicket/src/main/java/wicket/PageMap.java
           M /incubator/wicket/branches/wicket-1.2.x/wicket/src/main/java/wicket/Session.java
        
        clean up fix for dirty objects
        
        Show
        Jean-Baptiste Quenot added a comment - For reference this is the associated Subversion commit: r529917 | jcompagner | 2007-04-18 10:42:01 +0200 (Mer, 18 avr 2007) | 1 line Changed paths: M /incubator/wicket/branches/wicket-1.2.x/wicket/src/main/java/wicket/PageMap.java M /incubator/wicket/branches/wicket-1.2.x/wicket/src/main/java/wicket/Session.java clean up fix for dirty objects
        Hide
        Johan Compagner added a comment -

        not a complete backport, but fixed without api break.

        Show
        Johan Compagner added a comment - not a complete backport, but fixed without api break.
        Hide
        Johan Compagner added a comment -

        this fix can be done without any break of api if needed.

        Show
        Johan Compagner added a comment - this fix can be done without any break of api if needed.

          People

          • Assignee:
            Johan Compagner
            Reporter:
            Martijn Dashorst
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development