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

Ajax Session Driver - Allow Sessions to handle simultaneous requests #1900

Closed
wants to merge 5 commits into from

Conversation

Areson
Copy link

@Areson Areson commented Oct 17, 2012

This is in response to issue #154, and a rework of #1283 so that the changes are now bundled into a new session driver, tentatively called the "ajax" driver. For the sake of not having to link back, here is the overview of the changes, originally stated in #1283, with slight edits for clarity:

Overview

This update allows sessions to work gracefully with fast/simultaneous ajax requests without needing to simply ignore updating the session. Users are allowed to have more than one valid session id at any given time, but only one is considered to be "active." The active session id is allowed to be updated (e.g. generate a new session id, set user data, etc.), but all others session IDs associated with the user are marked as "inactive" and are read-only. Inactive sessions exist solely to make sure the requests that are created near the time a session is regenerated behave properly.

To facilitate this change an additional expiration time has been added, resulting in three "timeouts" for sessions:

  • Session Expiration: The point at which any request is destroyed
  • Time to Update: The point at which the active session id is regenerated
  • Multisession Expiration: The point which an inactive session is no longer valid. Any requests made with an inactive session id after this point are destroyed.

Here is an example of a session lifetime:

  • User interacts with the site for the first time, creating a new and active session, Session 1
  • After the Time to Update has passed, Session 1 is "updated." This process results in a new id being generated and associated with the new active session, Session 2. Session 1 is marked as inactive.
  • An AJAX request comes in with Session 1. Since it is marked as inactive, no update occurs, but the AJAX session completes because it has a valid session.
  • The multisession expiration passes
  • At this point, one of two final outcomes can happen:
    • Another AJAX request comes in with Session 1. Since the multisession expiration has passed, the session is destroyed and the AJAX request fails due to not having a valid session. (This is possible if the multisession expiration is set too low)
    • The session expiration passes for Session 1. The session will be destroyed during the course of the normal cleanup processes for sessions. Any request coming in for that session id will fail.

Changes from #1283/Important Notes

  • This driver requires the use of a database to back the sessions. As such, the option to enable/disable the database has been removed as an option for this driver. It is assumed to be available and the user only needs to provided the name of the table.
  • The multisession expiration (set by the option sess_multi_expiration) must be set sufficiently high so that any long running requests will finish before it expires. If it is set too short, dropped sessions may occur.
  • Enable simultaneous AJAX requests to work with sessions #1283 required an alteration to the session table structure. The ajax driver does not require this and will work with the current session table structure.
  • This driver does not provide any session locking outside what is needed to make sure that session regeneration happens properly. Simultaneous requests can still wipe out each other's data!
  • The driver provides a method, called multisession_expired which can be used to determine if the session associated with the request is inactive and therefore read-only

…ns were moved to the driver model.

Signed-off-by: Ian Oberst <areson@gmail.com>
…kie driver I eliminated the need to have a extra column in the session table for storing if the multisession expired. The ajax driver can no work as a drop-in driver with no database changes needed.

Signed-off-by: Ian Oberst <areson@gmail.com>
…ns were moved to the driver model.

Signed-off-by: Ian Oberst <areson@gmail.com>
…kie driver I eliminated the need to have a extra column in the session table for storing if the multisession expired. The ajax driver can no work as a drop-in driver with no database changes needed.

Signed-off-by: Ian Oberst <areson@gmail.com>
@narfbg
Copy link
Contributor

narfbg commented Oct 17, 2012

To be honest - I've always thought of CI's Session class to be way more complicated than it should be. This however is not complicated - it's simply wrong. Ajax is a client-side technology, you can't make a server-side "driver" for it.

@dchill42
Copy link
Contributor

To be fair, the name is somewhat misleading. I understand this to be a locking session driver aimed at eliminating most of the problems with concurrent or overlapping client requests (which often come via AJAX).

@GDmac
Copy link
Contributor

GDmac commented Oct 17, 2012

The latest session drivers don't regenerate a session_id during ajax requests.
If this still gives issues, please can you give an example?

Instead of an almost exact copy of the cookie driver, personally i'ld much rather have a fix
or mechanism that: on regenerate id, locks the session briefly and, passes the new
session_id on to next incoming requests somehow.

Race conditions are terrible to debug, or to be ready for or to prevent,
http://thwartedefforts.org/2006/11/11/race-conditions-with-ajax-and-php-sessions/

@Areson
Copy link
Author

Areson commented Oct 17, 2012

@narfbg I agree with @dchill42... the name is misleading. I more chose the name because it was meant to resolve the issue of dropped sessions that can occur when the session is regenerated during simultaneous AJAX requests, hence the "ajax" driver. I can certainly change the name of the driver.

@GDmac The current cookie driver doesn't regenerate a session_id during AJAX requests, and this does solve the problem of dropped sessions. However, this "bypass" of session regeneration makes things a bit more difficult when designing single page web applications, which is the case that I ran into. Since the code doesn't allow the session to regenerate during AJAX requests, I'm left with a few options:

  • Don't regenerate the session for the web application
  • Have the client side code periodically "pause" all outgoing AJAX requests and request that the session be regenerated
  • Have specific events in my server side code regenerate the session, preferably at times when I know that there won't be simultaneous requests

None of these options particularly thrill me, and they all force me to do additional work to simply get the session to regenerate. At the time, I chose to not use CodeIgniter's Session library and wrote my own that performed essentially the same function that this pull request does.

As for the near copy of the cookie driver, I wasn't sure if the people would prefer to have a separate driver so that the baseline cookie driver would be left alone, or if I should just modify the existing cookie driver, which would be most similar to what I had done originally in #1283. If the general feeling is that this should exist as modifications to the cookie driver rather than its own driver, I can certainly do that.

@daparky
Copy link
Contributor

daparky commented Oct 18, 2012

This is a very good addition. Even if this is 'right' it still needs to be resolved in the session drivers to allow AJAX.

@patricksavalle
Copy link

"This however is not complicated - it's simply wrong. Ajax is a client-side technology, you can't make a server-side "driver" for it."

All sessions are initiated client-side. And need a server-side solution.

I admit not having read all discussion about the AJAX session problems in CI, but isn't just a matter of locking and unlocking when updating? Making the session update a transaction? Looks to me like a very standard concurrency problem.

@Areson
Copy link
Author

Areson commented Oct 20, 2012

@patricksavalle It really is just a concurrency problem, and the current solution in CodeIgniter's session drivers is simply to not allow AJAX requests to cause the session to regenerate. As noted in my earlier response, this isn't always an acceptable solution.

However, the solution isn't as simple as just adding a lock around the session update. If your application can have multiple AJAX requests firing around the same time, then there is a potential for the session being dropped when it is update while other requests have already been sent to the server, even if you lock the session update. The situation would look something like this:

  • Request A with a session id of 123 is sent from the client
  • The session lock is initiated and the session is read on the server side
  • While the session is being read, Request B is sent from the client, also with a session id of 123
  • The session is finished being read for Request A is it is time to update the session.
    • The session is now 456 and 123 is no longer valid. A cookie is sent back to the client with the new session id of 456.
    • The session lock is released.
  • The session for Request B is locked and read in.
    • The session id associated with it, 123, is no longer valid.
    • The session driver generates a new session id of 789 and sends back a new cookie to the client. This cookie overwrites the 456 cookie, which means we no longer have a cookie for our actual session.
    • The session lock is released.
  • At this point the user would be forcibly logged since they no longer have a valid session.

Contrary to what the name of the pull request may lead people to believe (it was probably a poor name choice on my part) this is a server-side solution. It wraps the session regeneration in a lock, but also keeps the old session id around for a user specified amount of time. This allows any requests that were in flight at the time of the session update to be considered "valid" session and not drop the user. However, these "old" session ids are read-only, cannot cause the session id to be regenerated, and have a flag so that the programmer can take additional action if desired when this situation comes up. Once the user specified window for allowing these "old" session ids has passed, they are treated as expired sessions and will drop the user if they send a request with one. If the user specified window is set appropriately this will likely not happen at all.

@GDmac
Copy link
Contributor

GDmac commented Oct 20, 2012

This is not solely an ajax problem. I tested this with the following regular controler method. Whenever request A regenerates the session_id, then the waiting request B will operate on the old session_id. in the example i forcibly regenerate the id to show the issue.

Load this in a controller in two browser tabs, and load both within a sec. or two (sleep).

public function index()
{
    $this->load->library('session');
    $foo = $this->session->native->userdata('foo');
    $foo++;
    echo "<pre>{$foo}</pre>";
    $this->session->native->set_userdata('foo', $foo);
    $this->session->sess_regenerate();
    sleep(3);
}

@Areson instead of having multiple "valid" sessions id's, isn't there an elegant way to pass the new session_id (or actually, the whole dataset) to the waiting processes?

@GDmac
Copy link
Contributor

GDmac commented Oct 20, 2012

@Areson in your pull request you're using a native php session for locking, also i see it does a ini_set(session.use_cookies) without cleanup afterwards (what about using multiple drivers together? e.g. the native driver does need to set a cookie!). When using php native session already, why not work on the "native" driver then?

My proposal is to leave the original session_cookie driver for what it is for the time being,
(and keep it 100% backwards compatible), and do these changes on the new native session driver.

We could work on the php native driver to add database support in (via session_save_handler),
and add a more fine-grained locking control to it (database and/or file based).
(see linked article a few comments back),
and add a config option for session_save_path (for shared hosts) etc.

@Areson
Copy link
Author

Areson commented Oct 20, 2012

@GDmac In answer to your first question, I think there is, though I chose not to take that route because I was concerned about performance, as the solution I thought up requires at least one additional query to the database and/or a structure change. In essence, you could update the record for the old session id to include the ID of the new session. When reading in an old session, you could use that information to pull the data and ID from the new session. Unless you modify the structure of the database, this would likely require a second query when reading in an old session.

As to your second question, I don't believe my solution as it stands now would work using the native session handler. In order to prevent dropped sessions you have to control cookies fairly tightly in order to prevent a cookie with an older session id from being written. As I understand it, session_start sends a cookie to the client as part of its operation, so we have to know before starting the native session whether or not we want to send a cookie. This means we cannot prevent a cookie with an old session_id from being read in and overwriting the cookie with the new session id we previously sent.

Of course, you can always write out a new cookie once you realize you overwrote the new session id, but there are a couple of things that you'll need to overcome. First, we'll need database support for the native session in order somehow find the new session id. Second, overwriting the new session id on the client means that the client can send more requests with an old session id. This has the (unlikely?) potential of creating a self-sustaining problem where the cookie on the client flip-flops between the new session id and the old, and depending on which session id is in the cookie, cause dropped sessions.

The last problem is more of a security concern. Part of the reason I chose not to allow old sessions to write cookies out to the client was to avoid the issues stated above. The second is that allowing an old session id to give us back the new session id means that we've opened the door for someone to hijack a session with an old session id. This would be more or less of a concern depending on how we implemented our solution.

That's the cons. I do think we could take the route you are suggesting. Off the top of my head, it seems like we would have to pair the switch over to native sessions along with implementing the "elegant" passing of the new session id to the old session id. That, and adding DB support to native sessions, I think it would work. Something like this:

  • Read the session (cookie gets written)
  • Grab the session data from the database. If there is a newer session id, grab that id and the information
    • If the session is an older id, make sure it is within the timeframe allowed for getting the new session id. If not, kill it
    • If it is within the allowed timeframe, load the new session
  • Normal stuff

We'd have to have column in the database to store the reference to the new session id. That would also allow us read in the old and new session data in a single query when reading the session. When updating the session, we'd search for all of the rows that reference the old session id (in the "new session id" column) and point them at the new session id.

The only issue left that I haven't dealt with here is one that I mentioned earlier: the native session will write a cookie back to the client with an old session id when we initially read it. We'll overwrite it later, but that still means we open ourselves up to allowing the client to make more requests with the old session id. I think with what I outlined above that this generally wouldn't cause problems, but there is definitely the potential for dropped sessions if you have a fairly long sequence of rapidly send AJAX requests.

In all, I think I still prefer not using the native sessions, if only because I get better control over when and how cookies get sent to the client. Using something similar to the cookie session driver means we'll never have the cookie problem I mentioned above (though, it is debatable how much of a problem that would cause). But, I'm happy to go with what people feel like is going to be best for CI in general.

@dchill42
Copy link
Contributor

It sounds to me like this might be a feature worth including in the main driver library, with implementation details handled in individual drivers as necessary. I haven't drilled down into the mechanics yet, but I gather that it could be applied to either of the current backends - cookie and native.

Also, bear in mind that while session_start() does set a cookie, it can easily be negated if the associated session id is deemed invalid during processing. Look at the native implementation of sess_destroy() for an example. On top of that, I don't think the cookie data gets delivered to the client immediately, either. There is some flexibility in what the driver ultimately sends during the request.

@dchill42
Copy link
Contributor

See #1746 for some of the other discussion that has happened about session locking and its implications.

@narfbg
Copy link
Contributor

narfbg commented Dec 3, 2012

Closing this one, for numerous reasons:

  • Whatever feature this brings, it should be implemented in existing drivers and not as a separate one.
  • The problem that this PR aims to solve has only 1 real solution - read/write locks, anything else is a feaky hack (I've said this on another PR, not sure which one exactly).
  • This is actually more than a hack - it's labeled as a feature and yet it uses to its advantage a flaw that any security expert would say that is a vulnerability.

Nonetheless, thanks @Areson for the effort - it's not unnoticed, but it's not in the right direction - sorry.

@narfbg narfbg closed this Dec 3, 2012
@Areson
Copy link
Author

Areson commented Dec 3, 2012

@narfbg No worries! @dchill42 has been reworking session locking and forwarding in #1940, so hopefully a better solution that covers what I was going for will come out of that. Out of curiosity, what is the security flaw you mentioned?

@samson-htw
Copy link

The other freaky hack referenced might be #1713. I think the security flaw has to do with allowing a stale session id to access the current session, which potentially opens up the session to a third party using the stale session id.

@Areson
Copy link
Author

Areson commented Dec 3, 2012

@samson-htw That's the route that my mind went, though I wasn't sure if @narfbg had something different in mind.

@narfbg
Copy link
Contributor

narfbg commented Dec 3, 2012

Yep - you're enabling a race condition. If that was acceptable, we could just not regenerate the session id. :)

@8snf
Copy link

8snf commented Dec 20, 2012

Tip for those that still experience the problem like me.

http://blog.tiger-workshop.com/firephp-firefox-extension-causing-codeigniter-session-lost/

Firebug+FirePHP change the user agent when firebug is enabled/disabled and this causes session to die with little to no traces redirecting with header 302 to index.

This issue may be causing headache to others too.

@atticoos
Copy link

Yes - this is an incredible headache. Is there still no solution for this?

@Areson
Copy link
Author

Areson commented Feb 13, 2013

Unfortunately no. This pull request was not accepted and the other pull
request that was working on something similar (#1940) seems to have
stalled. You do have a couple of options:

On Tue, Feb 12, 2013 at 5:57 PM, ajwhite notifications@github.com wrote:

Yes - this is an incredible headache. Is there still no solution for this?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1900#issuecomment-13470783.

@atticoos
Copy link

@Areson Do you see any fallbacks with checking if the request is an ajax request and skipping $this->sess_update()?

@Areson
Copy link
Author

Areson commented Feb 13, 2013

You can do that. That was (is?) what Code Igniter sessions have setup as an
option. If the request is an ajax request just don't do any session
updating. This works pretty well if you have an application that consists
of multiple pages that the user will be navigating between, but in my
opinion this isn't great for applications that are essentially a single
page and dynamically load content.

On Tue, Feb 12, 2013 at 6:39 PM, ajwhite notifications@github.com wrote:

@Areson https://github.com/Areson Do you see any fallbacks with
checking if the request is an ajax request and skipping
$this->sess_update()?


Reply to this email directly or view it on GitHubhttps://github.com//pull/1900#issuecomment-13471826.

@atticoos
Copy link

@Areson I hear you on that, dynamically loaded single-page sites would defeat the purpose of this check, as we'd never reach a real session update.. Luckily, my situation just requires a lot of asynchronous loading of data into reporting tables and graphs that would normally kill load time.. So successive requests during this post page asynchronous loading of data have screwed me a few times during session refresh intervals.. I'll go ahead and add this little hack in there to ingore ajax requests. Thanks for the brief discussion.

@vancouverwill
Copy link

has anyone come up with any good tests to verify this does or doesn't happen. Currently I get the logging out issue super sporadically.
I have tried using brute force but that doesn't necessarily bring out the problem either. It would be good to prove it is happening so I can at least test on that.
thanks

@GDmac
Copy link
Contributor

GDmac commented Apr 4, 2013

test is: regenerating the session_id while other requests are still waiting or in the queue, these other requests use the old session_id, see test code in comment above 1900#issuecomment-9630963

(edit: either when session regenerate timeout kicks in, or when forcibly regenerating, as in the example code above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants