Language Change ? ? Pending

User tests: Successful: Unsuccessful:

avatar mavrosxristoforos
mavrosxristoforos
23 Dec 2022

Pull Request for Issue #39457.

Summary of Changes

Joomla gracefully handles errors and presents a nice interface to the admin with the ability to "return to dashboard". However, if the error is caused by a system plugin, the administrator interface becomes unusable.

Within the ExceptionHandler class, this pull request tries to identify a plugin directory from the error trace log, and provide a button for the admin to disable it. If the admin consents, the plugin is disabled.

The whole operation is completed within the ExceptionHandler class itself, because we are uncertain of whether the CMS can reach any other existing structure, since the plugin error is halting normal execution.

Testing Instructions

Edit any enabled legacy system plugin files and write erroneous code. This pull request should be able to handle parse errors and fatal errors, but not compile errors, which are unlikely for a published plugin. Warnings and notices are not caught by the Joomla exception handler, because the administrator remains usable.

Example of a parse error:

print 'a but no-semi-colon'
print 'b';

Example of a fatal error:

if (JFactory::getApplication()->isAdmin()) { return true; } // Deprecated since 2014 as Brian said!

Actual result BEFORE applying this Pull Request

Joomla shows the common error page and cannot recover by means of the administrator interface.

Expected result AFTER applying this Pull Request

A message should be enqueued and shown in the message area, with a button to disable the plugin. Since that sometimes fails, depending on how deep the app execution has gone before the plugin error, another disable button has been added in the Atum administrator error_full.php file, at the bottom, next to "Return to Dashboard".

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

avatar joomla-cms-bot joomla-cms-bot - change - 23 Dec 2022
Category Administration Language & Strings Templates (admin) Libraries
avatar mavrosxristoforos mavrosxristoforos - open - 23 Dec 2022
avatar mavrosxristoforos mavrosxristoforos - change - 23 Dec 2022
Status New Pending
avatar brianteeman
brianteeman - comment - 23 Dec 2022

image

image

avatar mavrosxristoforos
mavrosxristoforos - comment - 23 Dec 2022

It only works for legacy plugin events. Those are the main cause of the problem.

avatar brianteeman
brianteeman - comment - 23 Dec 2022

Then you need to update the test instructions and provide an example. I just tested exactly what you said to test

avatar brianteeman brianteeman - test_item - 23 Dec 2022 - Tested unsuccessfully
avatar brianteeman
brianteeman - comment - 23 Dec 2022

I have tested this item ? unsuccessfully on 94dd1eb


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39484.

avatar SharkyKZ
SharkyKZ - comment - 23 Dec 2022

Way to ignore the fact that there are valid reasons for plugins to throw exceptions.

avatar richard67
richard67 - comment - 23 Dec 2022

There are a lot of code style errors reported here by PHPCS: https://ci.joomla.org/joomla/joomla-cms/60302/1/6

avatar mavrosxristoforos mavrosxristoforos - change - 23 Dec 2022
The description was changed
avatar mavrosxristoforos mavrosxristoforos - edited - 23 Dec 2022
avatar mavrosxristoforos mavrosxristoforos - change - 23 Dec 2022
The description was changed
avatar mavrosxristoforos mavrosxristoforos - edited - 23 Dec 2022
avatar mavrosxristoforos
mavrosxristoforos - comment - 23 Dec 2022

Then you need to update the test instructions and provide an example. I just tested exactly what you said to test

Initial comment updated.

Way to ignore the fact that there are valid reasons for plugins to throw exceptions.

This is not a wrapper around plugin exceptions. It occurs when the administrator has failed and is wrapping up using the ExceptionHandler class as a last chance to show some interface. It only happens when a system plugin has crippled the administrator.

There are a lot of code style errors reported here by PHPCS: https://ci.joomla.org/joomla/joomla-cms/60302/1/6

Thank you, I will look at these.

avatar SharkyKZ
SharkyKZ - comment - 23 Dec 2022

This is not a wrapper around plugin exceptions. It occurs when the administrator has failed and is wrapping up using the ExceptionHandler class as a last chance to show some interface. It only happens when a system plugin has crippled the administrator.

It catches anything thrown by plugins, something that absolutely should not be done. It is not your concern what the plugins throws or why. This PR is conceptually wrong and should be closed.

avatar brianteeman
brianteeman - comment - 23 Dec 2022

I assume that this is the intended result ?

image

avatar mavrosxristoforos
mavrosxristoforos - comment - 23 Dec 2022

It catches anything thrown by plugins, something that absolutely should not be done.

Of course not, but in my tests it only catches what bricks your administrator until you manually fix it. Can you provide an example of what you're saying?

I assume that this is the intended result ?

Yes, Brian, indeed. Thank you. I am open to suggestions.

Still, I'm stressing out that we are losing people to this silly thing every day.

avatar richard67
richard67 - comment - 24 Dec 2022

It catches anything thrown by plugins, something that absolutely should not be done.

Of course not, but in my tests it only catches what bricks your administrator until you manually fix it.

@mavrosxristoforos Well, you catch any throwable. And that might include exceptions which should not be caught because the backend would still be bricked or malfunction when you don't catch them and disable a plugin because they are not caused by that plugin but by something under the hood. At least that's what I think @SharkyKZ could have in mind.

I have an idea how that could be sorted out, but I don't know if it's good or not.

Let's say we bring back the old methods isAdmin and isSite, but not as a proxy for the isClient calls (which never would work because e.g. !isAdmin() will never mean that it is site when mapping a boolean to a 4 state value). We let these methods just throw special, dedicated, new exceptions when they are called, nothing else. Then you don't just catch any callable where you do that right now, you catch only these special exception types.

This will limit the functionality added by this PR to foreseen cases (calls to the old isAdmin or isSite methods, probably also other cases could be implemented, too).

What do you guys and other readers think about that?

avatar mavrosxristoforos
mavrosxristoforos - comment - 24 Dec 2022

@richard67 Handling specific errors is definitely a good idea!
I think, though, that re-introducing isAdmin and isClient to return a different thing may become even more frustatring for extension developers. If it just throws a dedicated Exception, then that's ok IMO.

However, I feel I must insist that I am not handling every error here. Please read the rest of the ExceptionHandler class. This method is only invoked right before rendering the Joomla error screen from Brian's screenshot. The error log does not include plugin files unless the error is directly caused during their execution. I only address administrator errors that have a system plugin invoking them somewhere in the process. Can you provide any example in which this PR is handling legitimate errors that are not causing the whole administrator to crash?

Still, if we handle dedicated exceptions, that's ok with me. I am open to suggestions. I just want to see Joomla giving an option to admins that do not know how to manually fix their broken administrator.

avatar brianteeman
brianteeman - comment - 24 Dec 2022

What happens if you don't have permission to disable the plugin?

avatar laoneo
laoneo - comment - 27 Dec 2022

In general it is not bad to have some kind of safe mode. But catching everything is dangerous. The first use case which pops up in my mind is when a security plugin throws an exception because of malicious behavior. With this pr can then the bad user disable that security plugin. This should definitely not be possible.

avatar mavrosxristoforos mavrosxristoforos - change - 28 Feb 2023
Labels Added: Language Change ? ?
avatar brianteeman
brianteeman - comment - 17 Mar 2023

is this abandoned?

avatar mavrosxristoforos
mavrosxristoforos - comment - 17 Mar 2023

Not yet. I'm hoping to be able to commit an update soon. I have implemented all suggestions mentioned in this thread, but haven't been able to test them yet. I'm having trouble with re-setting up the test environment...

avatar nikosdion
nikosdion - comment - 17 Mar 2023

This is a very dangerous PR and should not even be considered.

Here are some real world failure modes I have seen:

  • A different 3PD plugin has replaced a core class (typically the database driver or the mailer), causing an innocent plugin to throw an exception.
  • A database issue —either caused by Joomla failing to apply the schema update, or a broken table in MySQL— causes an otherwise valid and tested SQL statement to fail.
  • Joomla failed to apply an update correctly, as @SniperSister has witnessed himself last year and I have been reporting since 2014 (and been seeing since 2005, all the while being called crazy…).
  • An OPcache screw-up —usually caused by Joomla not handling a PHP timeout during an extension update— results in an exception because the wrong version of the code is loaded. Bit me hard two years ago.
  • A security plugin blocking a malicious request which can only be done by throwing an exception. Do remember that the canonical way to tell Joomla to respond with a 403 HTTP status code is to throw a RuntimeException with a code of 403!

I could go on.

There are multiple problems with this approach.

First of all, regardless of whether the plugin was innocent or not, killing it instead of reporting the error makes it outright impossible to troubleshoot the problem. This is a very well-known, well-understood issue which plagues WordPress and makes our lives as developers miserable.

You will inevitably end up disabling a critical plugin which must never be disabled. For example, the core has WebAuthn, the Joomla user plugin, and the MFA plugins. Disabling them will end up making it impossible to log into the site, or degrade the site's security. In most real world sites there are critical third party plugins such as security plugins, sales processing, transactional email sending, extension frameworks, site builder plugins etc. Disabling any of these ends up acting as unattended bricking of the site. Yikes!

In most cases you will be killing an innocent plugin caught up in something which is not its fault, or one which deliberately throws an Exception to protect the site, and have Joomla slander it as broken. This opens OpenSourceMatters, Inc to legal liability for defamation and loss of profit.

Inevitably, us 3PDs will attempt to around the problem caused by Joomla by gimping the dangerously bad core feature, like we've done before (see Joomla Update and how it determines if our extensions have a migration path to a new Joomla version). In the case of this error handler we'll just add our own error handler around our plugin event handlers and classes which simply issues our own HTTP headers and message and then die()s. At best, this creates a jarring experience for users. At worst, when employed by 3PDs who don't have the same experience with core code and error handling me and other top developers have, will lead to broken sites e.g. if they register a global error handler while unregistering Joomla's built-in error handler.

My proposal

I am not completely against an error handler. I am against this implementation. An error handler, used wisely, can help with both objectives of a. troubleshooting error conditions and b. being able to operate the site in a "safe mode".

The current error handler (Symfony ErrorHandler) is great for backend developers but sucks for the moderately technical and non-technical site integrators and site owners, i.e. the majority of Joomla users.

The site integrator / owner wants to know which extension the error originated from so they can ask for support and, possibly, disable it temporarily until help comes their way.

I would propose displaying the following additional information in the error handler page:

  • The type and name of the extension where the error occurred
  • For plugins: which folder the extension is in, along with instructions on which entry point file to rename with a .bak extension (the services/provider.php for J4 native or the pluginname.php for J3 legacy) to temporarily disable the plugin.
  • For non-plugins: a direct link to the appropriate, filtered Manage Extensions page which allows you to disable the extension.
  • The link to the extension's developer (which is present in the XML manifest of the extension!) along with a simply worded instruction to contact this developer or the person responsible for the site for further support.

I will ask @joomla/joomla-experience-team-jxt to chip in because this is a very important user experience element. We want to improve UX for Joomla, not make people's lives harder.

avatar mavrosxristoforos
mavrosxristoforos - comment - 17 Mar 2023

@nikosdion Γεια σου Νίκο! So glad to read your ideas. Thank you for brainstorming us with all this valuable input on how to handle a REAL problem, that I feel as if I'm the only one facing with Joomla.

Let me point out once again, that this PR specifically handles SYSTEM plugins. Neither security plugins, nor user plugins.

However, this (what you wrote in your post) is exactly what I have been looking for, ever since starting issue #39457.
Still, it is important to state that this PR only ever occurs when the core has failed and is wrapping up just to show some interface. So, it only makes sense to assume that this interface must actually allow you to do something to solve the problem.

There are many valid reasons why an admin may not have FTP access to a website, and resorting to external tools to fix the problem is, still, IMHO bad UX...

avatar nikosdion
nikosdion - comment - 18 Mar 2023

@mavrosxristoforos Γειά σου, πατρίδα :)

Let me point out once again, that this PR specifically handles SYSTEM plugins. Neither security plugins, nor user plugins.

All the examples I had used are actually things I have seen implemented as system plugins

I can tell you with the authority of someone who's made and maintains a security extensions for Joomla that all security plugins —not just mine— are system plugins, by necessity. We have to run as early as we can during the Joomla application initialisation. This means system plugin and onAfterInitialise. We also need to handle just over 20 event types, making the system plugin a far more appealing solution than a scattering of 4–6 different plugins on top of the system plugin we need anyway.

Extension frameworks also tend to be system plugins because they need to do initialisation very early in the application boot up. One prime example, though I am not sure if it's still relevant in Joomla 4, is Koowa a.k.a. Nooku Framework.

For whatever reason (I have not taken on through a step by step debugger... yet!) page builder plugins and similar, uh, widget solutions (hint hint wink wink) are also system plugins. If these plugins are unpublished the frontend fails miserably and possibly uncovering some internals which should remain hidden.

Handling payment callbacks essentially requires having your own endpoint. Normally, this is through com_ajax and a plugin of a custom type. Some recalcitrant payment providers do not accept a URL with query string parameters (the previous Alpha Bank API in Greece for example, as I'm sure you know) so you either need a random .php file as your entry point (BAD!) or a system plugin which essentially handles its own, fixed route (e.g. /com_myecommerce/callback/weird_bank.html).

Plugins doing scheduled tasks such as sending emails also tend to be system plugins because the Scheduled Tasks feature is new, undocumented, and people still need to support as far back as Joomla 3.10 in the same codebase.

I have seen all of these on real world sites, I realise few core contributors have, that's why I raised the alarm ?

However, this (what you wrote in your post) is exactly what I have been looking for, ever since starting issue #39457.

Good!

There are many valid reasons why an admin may not have FTP access to a website, and resorting to external tools to fix the problem is, still, IMHO bad UX...

Yes, I thought about that too and I gave you half of the solution: the instructions to disable the extension includes a link to the extensions manager page.

Now, what about a failure which prevents that from working at all? I have a solution for that, but I did not explain it above because it's more involved. You're interested so let me go ahead.

As long as the logged in user has the core.edit_state privilege on com_plugins the instructions to disable the plugin can also include a single use, time-limited URL to disable the plugin. The idea is that upon hitting the error you create a secure token in #__user_keys with an expiration time 60 seconds into the future. When a special URL, let's say /administrator/index.php?__joomlaDisablePlugin=123&__token=blahblah is accessed, the AdministratorApplication removes the token, disables the referenced system plugin, and clears the system cache. You can ensure only the right plugin is removed by putting its ID in the token series, e.g. set the series to something like __joomla_error_admin_plugin_123. Therefore only the specific user, from the specific user agent, can disable a specific plugin, within a minute of the plugin throwing an error.

The observant reader may have recognised this approach as being similar to the way Admin Tools' Rescue Mode works to disable plg_system_admintools. This is indeed the case. What I am proposing is indeed something we have been successfully using in production in our own software the last 3 years. I didn't just pluck hair off my butt.

avatar HLeithner
HLeithner - comment - 2 May 2023

This pull request has been automatically rebased to 5.0-dev.

avatar HLeithner
HLeithner - comment - 30 Sep 2023

This pull request has been automatically rebased to 5.1-dev.

avatar HLeithner
HLeithner - comment - 24 Apr 2024

This pull request has been automatically rebased to 5.2-dev.

avatar HLeithner HLeithner - change - 24 Apr 2024
Title
Maintain usable admin on system plugin errors (Ref #39457)
[5.2] Maintain usable admin on system plugin errors (Ref #39457)
avatar HLeithner HLeithner - edited - 24 Apr 2024
avatar brianteeman
brianteeman - comment - 16 Jul 2024

This has obviously been abandoned and should be closed

avatar mavrosxristoforos
mavrosxristoforos - comment - 16 Jul 2024

Yes, I'm currently not working on it. I would appreciate any input to restart it. I believe I'm not the only one seeing the need to maintain a usable back-end.

avatar HLeithner
HLeithner - comment - 2 Sep 2024

This pull request has been automatically rebased to 5.3-dev.

avatar HLeithner HLeithner - change - 2 Sep 2024
Title
[5.2] Maintain usable admin on system plugin errors (Ref #39457)
[5.3] Maintain usable admin on system plugin errors (Ref #39457)
avatar HLeithner HLeithner - edited - 2 Sep 2024

Add a Comment

Login with GitHub to post a comment