@SniperSister @joomla/security flagging

Thanks! In general, I think that the risk for such a scenario is near zero - however it's perfectly fine to add it as a defence-in-depth hardening

Wouldn't this prevent store object of custom classes in session, mean potential backward incompatible change?

You mean when a dev does not store primitive values but complex objects in the session? Yes, indeed that would break.

You mean when a dev does not store primitive values but complex objects in the session? Yes, indeed that would break.

Yes. I think we should not accept this change because as you said: the risk for such a scenario is near zero and this could be a potential break for third party extensions.

That is a valid concern. Any extension that stores instances of custom
classes directly in the Joomla session (rather than primitive values or
arrays) would be affected, as those classes are not included in the
allowed_classes whitelist.

Would it be preferable to:

1. Accept this as a documented breaking change for 5.4 (with a note in
   the upgrade guide), or
2. Drop the allowed_classes restriction and keep only the instanceof
   guard + RuntimeException, which still hardens against a corrupted
   session without touching B/C?

That is a valid concern. Any extension that stores instances of custom
classes directly in the Joomla session (rather than primitive values or
arrays) would be affected, as those classes are not included in the
allowed_classes whitelist.

Would it be preferable to:

1. Accept this as a documented breaking change for 5.4 (with a note in
   the upgrade guide), or
2. Drop the allowed_classes restriction and keep only the instanceof guard + RuntimeException, which still hardens against a corrupted
   session without touching B/C?

That is a valid concern. Any extension that stores instances of custom classes directly in the Joomla session (rather than primitive values or arrays) would be affected, as those classes are not included in the allowed_classes whitelist.

Would it be preferable to:

1. Accept this as a documented breaking change for 5.4 (with a note in the upgrade guide), or
2. Drop the allowed_classes restriction and keep only the instanceof guard + RuntimeException, which still hardens against a corrupted session without touching B/C?

When the data can't be loaded we should not throw an exception without resetting the session data. The site will never load again when invalid data is stored. The visitor needs to clear the session cookie then in the browser dev tools. Not sure if we want that. Current behavior is that only the session data is lost which is enough in my opinion.

When the data can't be loaded we should not throw an exception without resetting the session data. The site will never load again when invalid data is stored. The visitor needs to clear the session cookie then in the browser dev tools. Not sure if we want that. Current behavior is that only the session data is lost which is enough in my opinion.

Valid point — throwing without resetting would leave the user in an unrecoverable loop.

Would it be acceptable to keep the allowed_classes restriction (for PHP Object Injection defense) but revert to the silent fallback: discard the invalid session data, reset to an empty Registry, and let the request continue? The user gets logged out rather than locked out, which is the current behaviour and is the right tradeoff.

The B/C concern about custom class objects in session would still apply to the allowed_classes restriction — happy to drop that too if the team prefers the instanceof guard alone.

Honestly I would leave it as is and close the pr.

Current behavior is that only the session data is lost which is enough in my opinion

It's not really true. When session data could not be unserialized for some reasons, we will soon get fatal error when there is code uses session API (because $this->data in the class is not a Registry object anymore)

I think the check + log like below code would be better:

 if (!empty($_SESSION['joomla'])) {
	$data = unserialize(base64_decode($_SESSION['joomla']));

	if ($data instanceof Registry) {
		$this->data = $data;
	} else {
		// Log error s
	}
}

Wouldn't this prevent store object of custom classes in session, mean potential backward incompatible change?

How would sessions actually store the data to a custom format? We would still accept the data in whatever format in the Registry object and with the close method isn't is always just a registry object? Like I'm not clear practically how a session could override during the closing of the session without fully replacing Joomla's session handler (and in that case they are overriding the start method too?) I'd say this seems like a reasonable sanity check.

@wilsonge I guess you mis-understood my comment. Yes, the whole session data is stored in a registry object. The problem is that we currently allow storing data in session not only for primitive data type such as int, string, array... but also objects from any PHP class.

The modification in this PR will only allows unserialize objects from Registry::class, \stdClass::class, so if someone store object of custom classes different from the two allowed, it could not be unserialized and will make the extension brokens. Even our core code will also broken because we store object of User class in session and that could not be unserialized with the changes in this PR.