Disable app that bricks the server after enabling #17451

PVince81 · 2015-07-07T15:32:52Z

If an app is getting enabled in the web UI, an ajax call is now made to
make sure the server still works. If it doesn't, it sends an emergency
app disabling call to disable the breaking app.

Fix #17411

@Raydiation @oparoz @LukasReschke

scrutinizer-notifier · 2015-07-07T15:32:59Z

A new inspection was created.

PVince81 · 2015-07-07T15:34:06Z

Note, this is a Proof Of Concept.

To test:

Setup this branch for core
Setup the calendar app, but use the "stable8" branch
Remove the version restriction in the calendar app's info.xml
Enable the calendar app in the web UI

Before this PR: it hangs, then OC is broken.
After this PR: app is disabled after the health check

PVince81 · 2015-07-07T15:34:47Z

lib/base.php

@@ -848,6 +848,17 @@ public static function handleRequest() {
 			self::checkUpgrade();
 		}

+		// emergency app disabling
+		if ($request === '/disableapp' && $_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['appid'])) {


I know, a bit barbaric... is there a better way ?
This has to happen before the first call to OC_App::loadApps

I think thats fine.

I'd suggest making it a GET parameter if possible, so the admin can open http://example.com/owncloud/disableapp?appid=broken-app manually if they refresh the page before the server health check completes (or if it breaks for some reason). Much easier than crafting a fake form to push a POST request.

@Xenopathic there are multiple browser addons that make POST a breeze.

GET is definitely wrong.

PVince81 · 2015-07-07T15:40:51Z

also @nickvergessen @icewind1991 @karlitschek @DeepDiver1975 for additional cheerfulness ? 😉

PVince81 · 2015-07-07T15:44:57Z

Fixes #17435

RobinMcCorkell · 2015-08-21T12:57:26Z

Fixes #18315

MorrisJobke · 2015-09-06T22:33:40Z

Alternative approach #18599

DeepDiver1975 · 2016-02-03T16:45:25Z

@PVince81 needs rebase - 9.0 or later? THX

nickvergessen · 2016-02-25T07:27:10Z

lib/base.php

@@ -848,6 +848,17 @@ public static function handleRequest() {
 			self::checkUpgrade();
 		}

+		// emergency app disabling
+		if ($request === '/disableapp' && $_SERVER['REQUEST_METHOD'] === 'POST' && isset($_POST['appid'])) {
+			\OCP\JSON::checkAdminUser();


Missing CSRF check?

PVince81 · 2016-02-25T10:01:54Z

Rebased and adjusted.
Added CSRF check.

Please review @MorrisJobke @nickvergessen @rullzer @schiesbn @blizzz

To simulate a broken app, just edit "user_ldap/appinfo/app.php" and add a syntax error. Then try enabling the app.

PVince81 · 2016-02-25T10:02:37Z

Make sure to use https://github.com/owncloud/core/pull/17451/files?w=1 for the review, the JS code was mostly indented.

MorrisJobke · 2016-02-25T17:25:27Z

Nice! Tested and works 👍

Also the code looks straight forward.

rullzer · 2016-02-25T20:26:18Z

Does not seem to work over here... the error page just returns code 200 here...

nickvergessen · 2016-02-29T08:23:35Z

lib/base.php

+		// emergency app disabling
+		if ($requestPath === '/disableapp'
+			&& $request->getMethod() === 'POST'
+			&& $request->getParam('appid') !== ''


default is null, which is !== '' so you will try to disable the app null, not sure what happens then, but should be adjusted:

((string) $request->getParam('appid')) !== ''

The casting to string is a good idea here anyway

Thanks for the info, I wasn't sure either. Usually I'd do a isset on $_POST

PVince81 · 2016-02-29T10:32:19Z

@nickvergessen I added the string casting.

nickvergessen · 2016-02-29T10:40:53Z

settings/js/apps.js

+								appId,
+								t('settings', 'Error: this app cannot be enabled because it makes the server unstable')
+							);
+							appItem.data('errormsg', t('settings', 'Error while disabling app'));


while enabling?

If an app is getting enabled in the web UI, an ajax call is now made to make sure the server still works. If it doesn't, it sends an emergency app disabling call to disable the breaking app.

PVince81 · 2016-02-29T11:08:08Z

@nickvergessen fixed the error handling

PVince81 · 2016-02-29T13:14:38Z

@nickvergessen second review ?

@rullzer can you elaborate how you tested this ?

nickvergessen · 2016-02-29T13:30:01Z

Works as expected, at least when the error is triggered in app.php or when displaying the files list.

👍

PVince81 · 2016-02-29T13:38:25Z

@rullzer mind giving it another test ? Would be good to know why it didn't work for you.

rullzer · 2016-02-29T13:41:32Z

@PVince81 still not working as expected.

Steps I took.

checkout branch 😉
navigate to app page
add some random noise into apps/user_ldap/appinfo/app.php
enable app

The app does not get disabled.

I see the ajax call being made. And the error page is returned. But it just has a http status code of 200.

PVince81 · 2016-02-29T13:45:15Z

@rullzer hmmm, maybe your random noise produced different errors.

I just added some text like "test" in app.php in the middle of the code, technically a syntax error.

PVince81 · 2016-02-29T13:47:19Z

I remember that there are some cases where ownCloud would just return an exception page, and that page would return the status code 200. Maybe my detection code isn't accurate enough.

Another idea would be to query the capabilities API, which I suppose should also cover many cases.

rullzer · 2016-02-29T13:48:32Z

@PVince81 I added "foo"... but that should not really matter

PVince81 · 2016-02-29T15:33:14Z

Ok got it, you added foo at the very top of the file.

I added it between some lines.

That causes another exception...

PVince81 · 2016-02-29T15:34:13Z

Actually, no. It doesn't brick ownCloud at all.
If you add foo before the <?php block then it's treated as content and likely ignored.

@rullzer please confirm / retest.

rullzer · 2016-02-29T18:53:37Z

@PVince81 no I added it in the actual php code.

But I don't think the issue here is with your code actually. The issue is that the error page return HTTP status code 200...

PVince81 · 2016-03-01T09:20:09Z

@rullzer please post the diff of your change and also the exception from the error page you see.

rullzer · 2016-03-01T09:56:41Z

The diff:

diff --git a/apps/user_ldap/appinfo/app.php b/apps/user_ldap/appinfo/app.php
index dab47ee..d744979 100644
--- a/apps/user_ldap/appinfo/app.php
+++ b/apps/user_ldap/appinfo/app.php
@@ -25,7 +25,7 @@
  */

 OCP\App::registerAdmin('user_ldap', 'settings');
-
+foobar
 $helper = new \OCA\user_ldap\lib\Helper();
 $configPrefixes = $helper->getServerConfigurationPrefixes(true);
 $ldapWrapper = new OCA\user_ldap\lib\LDAP();

Error stuff:

Internal Server Error

The server encountered an internal error and was unable to complete your request.

Please contact the server administrator if this error reappears multiple times, please include the technical details below in your report.

More details can be found in the server log.

Technical details

    Remote Address: 127.0.0.1
    Request ID: cvvZM3EB/y+YC1aFMNTk
    Type: ParseError
    Code: 0
    Message: syntax error, unexpected '$helper' (T_VARIABLE)
    File: /home/roeland/oc/core/apps/user_ldap/appinfo/app.php
    Line: 29


Trace

#0 /home/roeland/oc/core/lib/private/app.php(145): OC_App::requireAppFile('user_ldap')
#1 /home/roeland/oc/core/lib/private/app.php(118): OC_App::loadApp('user_ldap')
#2 /home/roeland/oc/core/lib/base.php(849): OC_App::loadApps(Array)
#3 /home/roeland/oc/core/index.php(39): OC::handleRequest()
#4 {main}

PVince81 · 2016-03-01T10:01:15Z

@rullzer very strange... I get a proper 500 Internal Server Error even with your patch. Possibly an env difference.

PVince81 · 2016-03-01T10:04:09Z

I applied this PR and your patch against 9.0beta2 and it also worked fine. Something seems wrong in your env.

Let's move forward and merge this.

Disable app that bricks the server after enabling

lock · 2019-08-07T10:02:26Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

PVince81 added the 2 - Developing label Jul 7, 2015

PVince81 mentioned this pull request Jul 7, 2015

Detect breaking apps when enabled in the web UI #17435

Closed

PVince81 reviewed Jul 7, 2015
View reviewed changes

nickvergessen mentioned this pull request Jul 20, 2015

[Apps Management] enabling an incompatible app results in broken installation #17411

Closed

PVince81 mentioned this pull request Aug 14, 2015

All apps get disabled on update #18312

Closed

RobinMcCorkell mentioned this pull request Sep 6, 2015

After upgrade to 8.1.1 enabling apps take down owncloud #18315

Closed

PVince81 added the settings:apps label Nov 20, 2015

nickvergessen reviewed Feb 25, 2016
View reviewed changes

PVince81 added this to the 9.0-current milestone Feb 25, 2016

PVince81 self-assigned this Feb 25, 2016

PVince81 force-pushed the apps-disablebrokenappafterenable branch from 840eb1a to 8835076 Compare February 25, 2016 10:00

PVince81 added 3 - To Review and removed 2 - Developing labels Feb 25, 2016

nickvergessen reviewed Feb 29, 2016
View reviewed changes

PVince81 force-pushed the apps-disablebrokenappafterenable branch from 8835076 to f0b6c81 Compare February 29, 2016 10:32

PVince81 added the sev2-high label Feb 29, 2016

nickvergessen reviewed Feb 29, 2016
View reviewed changes

Disable app that bricks the server after enabling

1dbe240

If an app is getting enabled in the web UI, an ajax call is now made to make sure the server still works. If it doesn't, it sends an emergency app disabling call to disable the breaking app.

PVince81 force-pushed the apps-disablebrokenappafterenable branch from f0b6c81 to 1dbe240 Compare February 29, 2016 11:07

PVince81 added 4 - To release and removed 3 - To Review labels Mar 1, 2016

DeepDiver1975 added a commit that referenced this pull request Mar 1, 2016

Merge pull request #17451 from owncloud/apps-disablebrokenappafterenable

256dfd2

Disable app that bricks the server after enabling

DeepDiver1975 merged commit 256dfd2 into master Mar 1, 2016

DeepDiver1975 deleted the apps-disablebrokenappafterenable branch March 1, 2016 10:26

PVince81 mentioned this pull request Oct 7, 2016

Improve app installation experience #8742

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019

Disable app that bricks the server after enabling #17451

Disable app that bricks the server after enabling #17451

Conversation

PVince81 commented Jul 7, 2015

scrutinizer-notifier commented Jul 7, 2015

PVince81 commented Jul 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jul 7, 2015

PVince81 commented Jul 7, 2015

RobinMcCorkell commented Aug 21, 2015

MorrisJobke commented Sep 6, 2015

DeepDiver1975 commented Feb 3, 2016

Choose a reason for hiding this comment

PVince81 commented Feb 25, 2016

PVince81 commented Feb 25, 2016

MorrisJobke commented Feb 25, 2016

rullzer commented Feb 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Feb 29, 2016

Choose a reason for hiding this comment

PVince81 commented Feb 29, 2016

PVince81 commented Feb 29, 2016

nickvergessen commented Feb 29, 2016

PVince81 commented Feb 29, 2016

rullzer commented Feb 29, 2016

PVince81 commented Feb 29, 2016

PVince81 commented Feb 29, 2016

rullzer commented Feb 29, 2016

PVince81 commented Feb 29, 2016

PVince81 commented Feb 29, 2016

rullzer commented Feb 29, 2016

PVince81 commented Mar 1, 2016

rullzer commented Mar 1, 2016

PVince81 commented Mar 1, 2016

PVince81 commented Mar 1, 2016

lock bot commented Aug 7, 2019