$session->destroy() and $session->stop() do not work?

[donpwinston]

I had to use session_destroy() which did work.

[lonnieezell]

Need more details. How did you start the session. Which driver were you using? etc...

Though it's possible just looking at the stop() function that it's missing a crucial piece. Still need the details to debug properly for you, though.

[ghost]

In my controller's constructor I have the following:
$this->session = \Config\Services::session();
$this->session->start();

I'm using the file system.
I've numerous routes that add to the session and
I've a route that simply clears the session and redirects to a page:

public function clear()
{
$this->session->stop();
//redirect('/register');
print "<pre>"; print_r($this->session->get()); print "</pre>";
}

[lonnieezell]

Ok, cool. Just making sure you weren't calling PHP's session_start anywhere, because that would screw things up.

I'll be honest, I'm familiar with the internals of the Session libs too much, I just ported over what CI3 had since it had just been rewritten by Narf. However, looking through the code here's what happens:

• sets the session cookie to an empty value
• regenerates the session_id so that the session is no longer in use
• in the FileHandler for this session, in the close() method, it releases the file lock and forgets all info about that.

Looking at the custom session handler docs the session is not completely destroyed until you specifically call it. In your example I believe the $_SESSION global would still be around, but no data would be in it anymore. It was never destroyed. Additionally, as long as the session classes have been loaded, the functions will be available to call.

What you didn't mention was whether the data was still around or not. It shouldn't be based on what I've seen in the code.

[ghost]

The data appears to still be there unless I call session_destroy() instead of $this->session->stop().

[lonnieezell]

That is the behavior I would expect based on those docs I linked to before. Sounds like it's all working as it should, then.

[Crenel]

I don't understand how leaving data intact after $session->stop() is correct functionality (i.e., "working as it should"). The CI 4 documentation clearly states that stop() can be used to destroy a session, but this is not happening. I don't know how long this may have been the case, but it's definitely true in 4.3.4. If I replace PHP's session_destroy() with CI4's $session>stop(), nothing happens to the session, all of the data remains - in my case, leaving me logged in despite attempting to log out. This represents a serious security risk.

[kenjis]

Don't use $session->stop() now. It seems broken.
It seems to be a bug.

[ahmetboz]

Is there any solution about that?

[Crenel]

This issue needs to be reopened (which I can't do), or a new one should be created that maybe references this one (which I'm not sure how to do).

"Just don't use it" is not a sufficient answer - this is a serious security flaw that will affect anyone who doesn't stumble across that bit of (critical!) advice.

If this github issue is any indication, this gaping hole has existed for years without anybody taking it seriously. That alone makes me question whether I should just abandon CodeIgniter (right when I was trying to build new systems on it). If the framework contributors are so disjointed that there is no cohesive team sense, or if there is a cohesive project team but they can't or won't recognize a major security flaw and fix it promptly, it's a lost cause.

Please... somebody... if there is someone / some team to which a severe security flaw can be escalated, get their eyes on this issue immediately.

[ahmetboz]

Is there any solution about that?

I've solved with session_destroy(); temporarily. But this case should be re-opened and the issue should be fixed.

[donpwinston]

I have no idea why there is a start and stop protocol to begin with. I'm sure almost no one uses it. All anyone needs is to create the session and destroy it. Maybe the idea was to "stop" updating the session data but not lose any data already stored in the session. Maybe the comment is wrong and not the code.

[lonnieezell]

I don't know the reasoning, since it was that way back in at least v3, if not earlier. Here's my guess, just based on reading through and previous discussions:

Narf always stressed that having the session open only when you need it is the safest thing to do, since for certain drivers it leads to file locking, temp files, etc. When a site is under attack, if a session is open for the entire length of the request than it can lead to out of storage errors. He often repeated that you should only have the session open when you actually need to use it for something. That's not the way a lot of us use sessions, but makes sense to me when thinking of scaling or the site being under attack.

When viewed in that context, it makes sense to stop a session when you don't need it anymore, which would release any file locks, let temp files be available to be freed, etc. And you wouldn't want to destroy the information in the session, just free up resources to be able to last a little longer during an attack or just a sudden influx of traffic. That's when stop() would be used, while destroy() would be reserved for when a user logs out or you simply don't need the information in the session any longer.

If the CI4 docs state otherwise, then the docs are in error here, it sounds like.

[lonnieezell]

I opened a new issue to update the docs, #7501

[Crenel]

Making this a documentation-only change leaves vulnerable any site built with the untested assumption that stop() will (for example) log somebody out (which it demonstrably does not do). So, either stop() needs to be deleted or renamed (intentionally breaking backward compatibility to "aggressively suggest" the use of session_destroy()), or it's purpose and functionality need to match the original documented purpose.

[kenjis]

@Crenel See #7503
We will merge it soon. If you have opinion, comment in the PR.