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

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

Closed
lonnieezell opened this issue May 15, 2023 · 7 comments · Fixed by #7503
Closed

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

lonnieezell opened this issue May 15, 2023 · 7 comments · Fixed by #7503
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@lonnieezell
Copy link
Member

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
Copy link
Member

kenjis commented May 16, 2023

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:

/**
* Does a full stop of the session:
*
* - destroys the session
* - unsets the session id
* - destroys the session cookie
*/
public function stop()

Implementation:

public function stop()
{
setcookie(
$this->sessionCookieName,
session_id(),
['expires' => 1, 'path' => $this->cookie->getPath(), 'domain' => $this->cookie->getDomain(), 'secure' => $this->cookie->isSecure(), 'httponly' => true]
);
session_regenerate_id(true);
}

@kenjis
Copy link
Member

kenjis commented May 16, 2023

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
Copy link
Member Author

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
Copy link
Member

kenjis commented May 16, 2023

fd59180 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
Copy link
Member Author

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
Copy link
Member

kenjis commented May 16, 2023

Okay, I will fix this bug.

@kenjis
Copy link
Member

kenjis commented May 17, 2023

Sent PR #7503

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them and removed dev labels May 17, 2023
@kenjis kenjis changed the title Dev: User Guide session->stop() docs are incorrect Bug: Session::stop() does not destroy Session May 17, 2023
@kenjis kenjis mentioned this issue May 18, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants