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

[5.3] Maintain usable admin on system plugin errors (Ref #39457) #39484

Open
wants to merge 2 commits into
base: 5.3-dev
Choose a base branch
from

Conversation

mavrosxristoforos
Copy link

@mavrosxristoforos mavrosxristoforos commented Dec 23, 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

@brianteeman
Copy link
Contributor

brianteeman commented Dec 23, 2022

image

image

@mavrosxristoforos
Copy link
Author

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

@brianteeman
Copy link
Contributor

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

@brianteeman
Copy link
Contributor

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.

@SharkyKZ
Copy link
Contributor

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

@richard67
Copy link
Member

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

@mavrosxristoforos
Copy link
Author

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.

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Dec 23, 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.

@brianteeman
Copy link
Contributor

I assume that this is the intended result ?

image

@mavrosxristoforos
Copy link
Author

mavrosxristoforos commented Dec 23, 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.

@richard67
Copy link
Member

richard67 commented Dec 24, 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?

@mavrosxristoforos
Copy link
Author

mavrosxristoforos commented Dec 24, 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.

@brianteeman
Copy link
Contributor

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

@laoneo
Copy link
Member

laoneo commented Dec 27, 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.

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jan 27, 2023
@brianteeman
Copy link
Contributor

is this abandoned?

@mavrosxristoforos
Copy link
Author

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...

@nikosdion
Copy link
Contributor

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.

@mavrosxristoforos
Copy link
Author

@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...

@nikosdion
Copy link
Contributor

@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.

@Hackwar Hackwar added the Feature label Apr 7, 2023
@HLeithner HLeithner changed the base branch from 4.2-dev to 5.0-dev May 2, 2023 16:41
@HLeithner
Copy link
Member

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

@alikon alikon removed the PR-4.2-dev label May 22, 2023
@HLeithner HLeithner changed the base branch from 5.0-dev to 5.1-dev September 30, 2023 22:50
@HLeithner
Copy link
Member

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

@HLeithner
Copy link
Member

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

@HLeithner HLeithner changed the base branch from 5.1-dev to 5.2-dev April 24, 2024 09:08
@HLeithner HLeithner changed the title Maintain usable admin on system plugin errors (Ref #39457) [5.2] Maintain usable admin on system plugin errors (Ref #39457) Apr 24, 2024
@brianteeman
Copy link
Contributor

This has obviously been abandoned and should be closed

@mavrosxristoforos
Copy link
Author

mavrosxristoforos commented Jul 16, 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.

@HLeithner HLeithner changed the base branch from 5.2-dev to 5.3-dev September 2, 2024 08:53
@HLeithner
Copy link
Member

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

@HLeithner HLeithner changed the title [5.2] Maintain usable admin on system plugin errors (Ref #39457) [5.3] Maintain usable admin on system plugin errors (Ref #39457) Sep 2, 2024
@Hackwar Hackwar removed the PR-5.2-dev label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Change This is for Translators PR-5.3-dev Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.