Paging @joomla/security

JSST, you all pushed for this, someone should testing this. Or is this another "it's a Michael item, ignore it since he's on the Joomla community blacklist for being too blunt" thing?

@mbabker do you have a link to this blacklist?

anyway no it's not ignore it's on my list, please keep it open.

I have tested this item ✅ successfully on d97b9d0

played with this pr on for a bit ... All the session data is always there.

I have tested this item ✅ successfully on d97b9d0

played with this pr on for a bit ... All the session data is always there.

If you fix the conflicts here i'll merge it

Begrudgingly rebased to prevent more possible bad ideas from floating around since the session code isn't fully understood by anyone.

From a JSST perspective, as I understand it, session meta-data is only useful for logged-in users, so that those sessions can be force-cleared if needed.

However, I don't see a reason to revert the change in #13322 as long as sessions meta-data are ON by default and optional to turn off (and user knowingly abandoning the corresponding features to gain scalability).

However, currently in 3.9, when non-database (e.g. PHP) sessions are selected, each anonymous guest still creates a session meta-data table entry, which is not very useful (at least from a JSST perspective as I understand it) and makes the table grow at each new access, which is a potential JSST issue.

I am still wondering what other use, apart of being able to count guests, guest sessions meta-data have ?

I am still wondering what other use, apart of being able to count guests, guest sessions meta-data have ?

That's basically it. Plus, in 3.x, the database session handler has no concept of "create if does not exist", it relies on the metadata manager to insert the record into the database for the session handler to update it when the session is written. I fixed that fatal flaw in the Framework package that is used for the 4.0 API, which is in part why you can actually turn off metadata now.

The metadata manager has no concept of "only write for authenticated sessions". You can add an event in the manager to let someone control this behavior, but because the manager is executed before any plugin is dispatched, that's not possible. It's part of the reason I feel like the "Access" field in the plugin manager needs to go away (plugins should always load, not load based on user's groups, because in the case of a number of plugins (like those which impact output, i.e. {loadmodule} type shortcodes) they need to handle the not allowed part of the ACL and in the current state they can't; remove that filter from the query and Joomla\CMS\Plugin\PluginHelper::load() doesn't arbitrarily force a session to be started and you might actually be able to dispatch the currently flawed onBeforeExecute event without a session being started and gain a lot of control over that aspect of core without coming up with "creative" workarounds if you're deploying sites that have no point in having frontend sessions).

At the time of this writing downloads.joomla.org has a session table of 2374 rows with 1313 of those created by the API application, which is stateless by design, with the other thousand rows being for the frontend which has no need for sessions (and given the nature of how core updates run, downloading the package routes through the CMS to get the signed AWS URL which means a session is started on that request), and there is nobody logged into the admin. This is in part why there needs to be able to be a killswitch on metadata, and any feature that relies on this metadata tracking ability (in core or extensions) should respect the killswitch, and why I was vehemently against the ideas floating around in JSST to add another metadata type data bit to bypass being able to turn this off. I also agree with the stance a lot of people have that it should be possible to disable sessions in Joomla for the frontend (though for architectural and performance reasons, not so much the GDPR related stuff people like to throw around with the session cookie). But I don't have the energy to champion that fight anymore and I hope whomever does also has a good therapist on-call.

What you say shows that there is an inconsistency in the way system plugins are handled, and makes full sense.

Imho system plugins should not be subject to ACL for loading, and those like the "debug" one could check ACL later in execution. Would that solve the issue ?

And btw why would ACL need to create a persistent guest session to check if there is an authenticated session ? Would that mostly solve the issue ?

Actually, in most cases we don't even need a session for guests ? So maybe have a setting to turn off auto-creation of guest sessions ?

Thus, I don't see a reason for this PR nor any reason to revert #13322 as the setting added is ON by default and description warns of some features not working if turned off.

Now-- that I better understand the issue, if my vote counts, please take this is as a "Don't merge this PR" vote.

EDIT: striked through last sentence.

At @HLeithner ‘s request today on glip sitting on this for a week. Unless I hear definitely otherwise will merge next weekend

You can’t distinguish “guest” session with “authenticated” session without
starting a session. Or with cookies. So, what you’re suggesting is not
architecturally feasible. Plus, without an already started session, any
functionality that uses CSRF (login form, contact form) is busted; that’s a
bit of a chicken versus egg scenario.

System plugins aren not handled differently than other plugin groups, nor
should they be. The only proper fix to the plugin ACL issue is to remove
the access group filter in the query that determines all of the plugins
that can be loaded in a request (as that query is run once on the first
import of a plugin group). That comes with a B/C break of every plugin
then being responsible for its own ACL decisions instead of core being able
to decide “do not load this plugin at all”.

The metadata issue is really just a minor performance issue with small
impact on a couple of core features. The actual session management is a
bit deeper of an issue and probably could’ve been iterated over before
4.0’s release if we really identified all of the problems in 2016 and
started work then. It’s too late IMO to tackle it now unless someone can
work out a mostly B/C path forward.

On Sat, Jun 8, 2019 at 10:20 AM beat notifications@github.com wrote:

What you say shows that there is an inconsistency in the way system
plugins are handled, and makes full sense.

Imho system plugins should not be subject to ACL for loading, and those
like the "debug" one could check ACL later in execution. Would that solve
the issue ?

And btw why would ACL need to create a persistent guest session to check
if there is an authenticated session ? Would that mostly solve the issue ?

Actually, in most cases we don't even need a session for guests ? So maybe
have a setting to turn off auto-creation of guest sessions ?

Thus, I don't see a reason for this PR nor any reason to revert #13322
#13322 as the setting added is
ON by default and description warns of some features not working if turned
off.

Now that I better understand the issue, if my vote counts, please take
this is as a "Don't merge this PR vote".

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/pull/24765?email_source=notifications&email_token=AACZ7IJUPAPFDQIGEPNKPALPZPE2DA5CNFSM4HJLAM6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXHWSRI#issuecomment-500132165,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AACZ7IPPGTIWUKRNMIE3BSLPZPE2DANCNFSM4HJLAM6A
.

--

• Michael Please pardon any errors, this message was sent from my iPhone.

You can’t distinguish “guest” session with “authenticated” session without starting a session. Or with cookies. So, what you’re suggesting is not architecturally feasible.

Correct for full session verification. However if there are no session cookies at all, one can reasonably assume that it is a guest. I don't see an architectural impossibility there.

Plus, without an already started session, any functionality that uses CSRF (login form, contact form) is busted; that’s a bit of a chicken versus egg scenario.

Indeed on such pages with forms you need a session started. But not all pages on all sites have such forms on every page. That's typicaly the case for mostly-read-only sites that need scalability.

So even while keeping B/C for plugins, we can still delay starting of guest sessions until needed.

This is just the tip of the sessions iceberg and probably too late to change for 4.0.

You cannot distinguish between a "guest" session and an "authenticated" session. There is no concept of such in PHP's session extension. The ability to make that distinction relies on data either stored in the session (and in the case of Joomla logged separately in the database, AKA the session metadata) or stored elsewhere in a way that emulates a session state (i.e. a cookie).

You cannot delay starting a session until absolutely needed based on the presence of a cookie, or if you really go crazy with your PHP configuration a parameter read from $_GET. The Joomla API is already lazy in that the first call to Joomla\Session\Session::(all|clear|get|has|remove|set) will start the session (no way to enforce this as an interface behavior though) and there is no sane way around that behavior without removing support for lazily loaded sessions and require an explicit call to Joomla\Session\SessionInterface::start() which is IMO a bad idea (in the context of the CMS it would have to be called when creating the session service, which was the case before roughly 3.7 when I adapted the session API to support lazy start which moved the start point out of the application class constructor and into the application's execute() method).

Anything that tries to be "super lazy" will end up most likely starting and destroying multiple sessions in a single request cycle, which has problems of its own to deal with and a major performance penalty. The absolute best you can do is get rid of every session related dependency it takes to execute the currently flawed onBeforeExecute event, that means getting rid of the "Access" field on the plugin manager and the related filter in the query. Once that is done, a plugin can implement a solution that has more control over sessions than what exists now.

Ok, then I hope that I understand better how far we are from getting sessions-free (and cookies-free) and that it is way too late for 4.0 to change all of that. And I don't see a way with B/C breaking for now.

Thus I removed my vote against this PR above to "neutral".

What might be of interest would be to convert the meta-session setting that this PR would be removing to (for now) a setting that only stores meta-sessions for authenticated users sessions (basically balancing the number of unneeded meta-sessions rows and guests-counting) ?

What might be of interest would be to convert the meta-session setting that this PR would be removing to (for now) a setting that only stores meta-sessions for authenticated users sessions (basically balancing the number of unneeded meta-sessions rows and guests-counting) ?

If someone decides to go that route then it should be an option to either completely disable it, log only for authenticated users, or log for all users. It shouldn't be an either/or choice. Personally, I don't see the benefit to disabling metadata for only guests and logging it for authenticated users; either you're going to want the metadata or you're not, just thinking from the top of my head there are few cases where logging the metadata only for authenticated users is what you're actually going to want for your site.

At least the the access check for plugins could be moved there without b/c at least I don't see any.

#25152

Would also not revert to disable the metadata collection, in this case you can do it in the cms, but you could do it with your own MetadataManager but then you can't communicate the side affects to the user.

Communicating to the user that disabling this feature would also disable security features (like destroy all user sessions on password changed) should be enough.

It would be bad for the performance for many sites not using the database as session handler and tries to have 0 database queries on cached requests. I personally always apcu as caching layer, having metadata in the database would simple be.... useless specially if the metadata is not used at all.

For the CSRF problem it's maybe not necessary to use a session, this could be done with an own cookie too.

At least the the access check for plugins could be moved there without b/c at least I don't see any.

Architecturally there is no B/C break. Fundamentally there is given that plugins (and probably mambots before that) were designed under the assumption that a user may control which user groups are allowed to load and execute the plugin, and now ACL related checks are now the sole responsibility of plugins.

Personally, I'm not fond of the changes in the ExtensionManagerTrait in #25152 because it still creates a scenario where the plugin doesn't load at all. And that's still problematic; there are going to be cases where a plugin needs to undo something if ACL fails (again, the "replace shortcode in an article with an empty string" example fits this perfectly), so core should not make the assumption of "Access is set to 'Registered', this plugin should not load at all for guests". The rest of it I think can work.

With this PR its possible for the plugin to disable or bypass the ACL because it has full control.
PlgContentLoadmodule would be loaded and can decide if it will execute the event code on each call.

As you say your self the cms should not decide if a plugin should be loaded or not, with the PR we give the plugin the control and responsibility.

I'm open for every suggest you have on the pr also because I would like to have the possibility to view a page without any database call. I'm not sure if this is really possible but with caching and the plugin pr we are down to 4 queries.

• sql_mode
• 2 access queries for the user by the menu
• And version check from the library?

UPDATE `ds29v_extensions`
SET `params` = '{\"mediaversion\":\"c463da2fbaae6d1cddd3552e989e6797\"}'
WHERE `type` = 'library' AND `element` = 'joomla'

This update is done in every call? Ah ok I cached it, it will be called if debugging is active.

So removing the menu access check or better try to make it a bit smarter may would bring us to 0 queries.

That query for the media version is done when debug is enabled.

I commented on the PR where specifically the "plugin doesn't load at all" issue can come up.