Bug: Session::stop() does not destroy Session

[lonnieezell]

The user guide incorrectly states that ->stop() destroys the session. It does not. In the implementation it updates the cookie to close the session ASAP and regenerates the session ID to protect against session fixation attacks. It does not destroy the session. The docblock there also incorrectly states that it destroys the session that should be removed.

Stopping a session allows someone to close the session without losing any data that might be currently stored in the session.

Looks like this was incorrectly labeled in the docblock in #4771 .

See discussion on #592

[kenjis]

To be honest, I don't understand why this method exists and implemented like this.

User Guide:

You may also use the stop() method to completely kill the session by removing the old session ID, destroying all data, and destroying the cookie that contained the session ID: https://codeigniter4.github.io/CodeIgniter4/libraries/sessions.html#destroying-a-session

Doc comment: https://github.com/codeigniter4/CodeIgniter4/blob/fb0583f1f736f0b4720e3171a774a1883b292930/system/Session/Session.php#L274-L281

Implementation: https://github.com/codeigniter4/CodeIgniter4/blob/fb0583f1f736f0b4720e3171a774a1883b292930/system/Session/Session.php#L281-L290

[kenjis]

In the current implementation, the setcookie() in the method does not work at all. And all session data remains. The stop() is equivalent to just calling session_regenerate_id(true).

diff --git a/app/Config/Routes.php b/app/Config/Routes.php
index c251ec22c4..112271665c 100644
--- a/app/Config/Routes.php
+++ b/app/Config/Routes.php
@@ -19,7 +19,7 @@ $routes->set404Override();
 // where controller filters or CSRF protection are bypassed.
 // If you don't want to define all routes, please use the Auto Routing (Improved).
 // Set `$autoRoutesImproved` to true in `app/Config/Feature.php` and set the following to true.
-// $routes->setAutoRoute(false);
+$routes->setAutoRoute(true);

 /*
  * --------------------------------------------------------------------
diff --git a/app/Controllers/Home.php b/app/Controllers/Home.php
index 7f867e95ff..2745406a20 100644
--- a/app/Controllers/Home.php
+++ b/app/Controllers/Home.php
@@ -6,6 +6,17 @@ class Home extends BaseController
 {
     public function index()
     {
-        return view('welcome_message');
+        $session = session();
+        $session->set('foo', 'bar');
+
+        d($session->get(), $_SESSION);
+    }
+
+    public function stop()
+    {
+        $session = session();
+        $session->stop();
+
+        d($session->get(), $_SESSION);
     }
 }

Navigate to home:

{
    "Response Headers (446 B)": {
        "headers": [
            {
                "name": "Cache-Control",
                "value": "no-store, no-cache, must-revalidate"
            },
            {
                "name": "Cache-Control",
                "value": "no-store, max-age=0, no-cache"
            },
            {
                "name": "Connection",
                "value": "close"
            },
            {
                "name": "Content-Type",
                "value": "text/html; charset=UTF-8"
            },
            {
                "name": "Date",
                "value": "Tue, 16 May 2023 03:03:16 GMT"
            },
            {
                "name": "Expires",
                "value": "Thu, 19 Nov 1981 08:52:00 GMT"
            },
            {
                "name": "Host",
                "value": "localhost:8080"
            },
            {
                "name": "Pragma",
                "value": "no-cache"
            },
            {
                "name": "Set-Cookie",
                "value": "ci_session=91ikuem4clgojhueua4qunhm36be7ae1; expires=Tue, 16-May-2023 05:03:16 GMT; Max-Age=7200; path=/; HttpOnly; SameSite=Lax"
            },
            {
                "name": "X-Powered-By",
                "value": "PHP/8.1.18"
            }
        ]
    }
}

Navigate to home/stop:

{
    "Response Headers (446 B)": {
        "headers": [
            {
                "name": "Cache-Control",
                "value": "no-store, no-cache, must-revalidate"
            },
            {
                "name": "Cache-Control",
                "value": "no-store, max-age=0, no-cache"
            },
            {
                "name": "Connection",
                "value": "close"
            },
            {
                "name": "Content-Type",
                "value": "text/html; charset=UTF-8"
            },
            {
                "name": "Date",
                "value": "Tue, 16 May 2023 03:03:40 GMT"
            },
            {
                "name": "Expires",
                "value": "Thu, 19 Nov 1981 08:52:00 GMT"
            },
            {
                "name": "Host",
                "value": "localhost:8080"
            },
            {
                "name": "Pragma",
                "value": "no-cache"
            },
            {
                "name": "Set-Cookie",
                "value": "ci_session=iji741a2qesbj2m8in5j7tsjig3f6pq7; expires=Tue, 16-May-2023 05:03:40 GMT; Max-Age=7200; path=/; HttpOnly; SameSite=Lax"
            },
            {
                "name": "X-Powered-By",
                "value": "PHP/8.1.18"
            }
        ]
    }
}

[lonnieezell]

Looks like I added these years ago but I have no idea why, honestly. The PR makes it seem like a purely semantic thing.

I could have sworn it was in the port from CI3, but turns out that's not the case.

Seems my previous reasoning on why this existed was likely incorrect. In this case I think we should call it a bug, and simply make it an alias for destroy().

What do you think @kenjis ?

[kenjis]

https://github.com/codeigniter4/CodeIgniter4/commit/fd59180ce136a9c01b502908f987eb3600bcaa65 says "Updating session library to have manual start and stop methods." I'm not sure the meaning of stop, but it may be close in the meaning of session-write-close. But then the explanation for stop() in the User Guide is too different.

Now no one knows why it was implemented that way, so it's not worth worrying about.

In my opinion, I think the following is fine:

1. Deprecate stop() as it is not needed
2. Make stop() an alias for destory() to destroy session (because the UG says)
3. Add close() which calls session_write_close()

(3) is not directly relevant. So it will be starting with 4.4. And we will be able to start and close (stop) session manually.

Or if we really need a method to completely kill the session, we should implement a method like the following:

        // remove all session data
        $_SESSION = [];

        // delete the session cookie
        setcookie(
            $this->sessionCookieName,
            session_id(),
            [
                'expires'  => 1,
                'path'     => $this->cookie->getPath(),
                'domain'   => $this->cookie->getDomain(),
                'secure'   => $this->cookie->isSecure(),
                'httponly' => true,
            ]
        );

        session_destroy();

But the method name should not be stop(). Because I don't think stop means destroying the session, I think it is possible to start again after stopping.

[lonnieezell]

I don't think we need something to completely kill the session, but a close method would probably be helpful.

I agree with your plan.

[kenjis]

Okay, I will fix this bug.

[kenjis]

Sent PR #7503