From 6910328a46955851e41e4771a78b913db943d163 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Tue, 12 Nov 2019 11:01:47 +0100 Subject: [PATCH 1/5] Set 500 HTTP code on error --- index.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/index.php b/index.php index 0e36dddc8ed6..03dad6ff177e 100644 --- a/index.php +++ b/index.php @@ -77,6 +77,9 @@ } catch (\Exception $ex2) { // with some env issues, it can happen that the logger couldn't log properly, // so print out the exception directly + // NOTE: If we've reached this point, something has gone really wrong because + // we couldn't even get the logger, so don't rely on ownCloud here. + \http_response_code(500); echo(''); echo('Exception occurred while logging exception: ' . $ex->getMessage() . '
'); echo(\str_replace("\n", '
', $ex->getTraceAsString())); From 9111ba0127f16a64e8e50693e1dc2c40053141e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Tue, 12 Nov 2019 13:53:09 +0100 Subject: [PATCH 2/5] Rewrite favicon.ico to the right location This could happen if the server crashes using the web UI. The browser sends a request to the /favicon.ico URL instead of /core/img/favicon.ico, which is configured in the ownCloud's web pages. Without this fix, the server responds with a 500 instead of a 200 or 404 --- lib/private/Setup.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/Setup.php b/lib/private/Setup.php index ce6a05697740..a93ab4b8d11c 100644 --- a/lib/private/Setup.php +++ b/lib/private/Setup.php @@ -480,6 +480,7 @@ public static function updateHtaccess() { if ($rewriteBase !== '') { $content .= "\n"; $content .= "\n Options -MultiViews"; + $content .= "\n RewriteRule ^favicon.ico$ core/img/favicon.ico [L]"; $content .= "\n RewriteRule ^core/js/oc.js$ index.php [PT,E=PATH_INFO:$1]"; $content .= "\n RewriteRule ^core/preview.png$ index.php [PT,E=PATH_INFO:$1]"; $content .= "\n RewriteCond %{REQUEST_FILENAME} !\\.(css|js|svg|gif|png|html|ttf|woff|ico|jpg|jpeg|json)$"; From faddb892c325f732e31e80f30e652b123cedcc27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Tue, 12 Nov 2019 17:57:21 +0100 Subject: [PATCH 3/5] Remove onShutdown error handler It was causing an additional DBALException when there is no DB connection. The logger was trying to access to the DB during the shutdown process, causing an unhandled exception to appear in apache --- lib/private/Log/ErrorHandler.php | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/lib/private/Log/ErrorHandler.php b/lib/private/Log/ErrorHandler.php index ad5c70624b18..7c0075cc786d 100644 --- a/lib/private/Log/ErrorHandler.php +++ b/lib/private/Log/ErrorHandler.php @@ -50,9 +50,6 @@ public static function register($debug=false) { } else { \set_error_handler([$handler, 'onError']); } - \OC::$server->getShutdownHandler()->register(function () use ($handler) { - $handler->onShutdown(); - }); \set_exception_handler([$handler, 'onException']); } @@ -60,16 +57,6 @@ public static function setLogger(ILogger $logger) { self::$logger = $logger; } - //Fatal errors handler - public static function onShutdown() { - $error = \error_get_last(); - if ($error && self::$logger) { - //ob_end_clean(); - $msg = $error['message'] . ' at ' . $error['file'] . '#' . $error['line']; - self::$logger->critical(self::removePassword($msg), ['app' => 'PHP']); - } - } - /** * Uncaught exception handler * From 9e1b300512c5af6e09919a6dae65603e37e5b933 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Wed, 13 Nov 2019 15:12:33 +0100 Subject: [PATCH 4/5] Use a different logger and log file if ownCloud is broken The endpoints will return a 599 HTTP status to distinguish app failures, which shouldn't break ownCloud and can still be reported through normal means. It will use a different file to log what happened without messing with the owncloud.log file and without bothering the web server's log Include new configuration option in config.sample.php file --- config/config.sample.php | 11 +++++++++ console.php | 20 ++++++++++----- index.php | 16 ++++-------- lib/base.php | 53 ++++++++++++++++++++++++++++++++++++++++ public.php | 28 +++++++++++---------- remote.php | 13 +++++++--- status.php | 13 +++++++--- 7 files changed, 117 insertions(+), 37 deletions(-) diff --git a/config/config.sample.php b/config/config.sample.php index b76335f71927..94bd3e64efe5 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -82,6 +82,17 @@ */ 'datadirectory' => '/var/www/owncloud/data', +/** + * Define the directory where the crash logs will be stored. + * By default, this will be the same as the one configured as "datadirectory". + * The directory MUST EXIST and be WRITABLE by the web server. + * Note that crashes are extremely rare (although they can come in burst due to + * multiple requests), so the default location is usually fine. + * Also note that the log can contain sensitive information, but it should be useful + * to pinpoint where is the problem. + */ +'crashdirectory' => '/var/www/owncloud/data', + /** * Current version number of your ownCloud installation * This is set up during installation and update, so you shouldn't need to change it. diff --git a/console.php b/console.php index 92365fa227a6..72f139f7639d 100644 --- a/console.php +++ b/console.php @@ -56,9 +56,19 @@ } function exceptionHandler($exception) { - echo 'An unhandled exception has been thrown:' . PHP_EOL; - echo $exception; - exit(1); + try { + // try to log the exception + \OC::$server->getLogger()->logException($ex, ['app' => 'index']); + } catch (\Throwable $ex) { + // if we can't log normally, use the crashLog + \OC::crashLog($exception); + \OC::crashLog($ex); + } finally { + // always show the exception in the console + echo 'An unhandled exception has been thrown:' . PHP_EOL; + echo $exception; + exit(1); + } } try { require_once __DIR__ . '/lib/base.php'; @@ -104,8 +114,6 @@ function exceptionHandler($exception) { $application = new Application(\OC::$server->getConfig(), \OC::$server->getEventDispatcher(), \OC::$server->getRequest()); $application->loadCommands(new ArgvInput(), new ConsoleOutput()); $application->run(); -} catch (Exception $ex) { - exceptionHandler($ex); -} catch (Error $ex) { +} catch (\Throwable $ex) { exceptionHandler($ex); } diff --git a/index.php b/index.php index 03dad6ff177e..3621bc3e5343 100644 --- a/index.php +++ b/index.php @@ -67,26 +67,20 @@ } catch (\OCP\Files\ForbiddenException $ex) { OC_Response::setStatus(OC_Response::STATUS_FORBIDDEN); OC_Template::printErrorPage($ex->getMessage()); -} catch (Exception $ex) { +} catch (\Throwable $ex) { try { \OC::$server->getLogger()->logException($ex, ['app' => 'index']); //show the user a detailed error page OC_Response::setStatus(OC_Response::STATUS_INTERNAL_SERVER_ERROR); OC_Template::printExceptionErrorPage($ex); - } catch (\Exception $ex2) { + } catch (\Throwable $ex2) { // with some env issues, it can happen that the logger couldn't log properly, // so print out the exception directly // NOTE: If we've reached this point, something has gone really wrong because // we couldn't even get the logger, so don't rely on ownCloud here. - \http_response_code(500); - echo(''); - echo('Exception occurred while logging exception: ' . $ex->getMessage() . '
'); - echo(\str_replace("\n", '
', $ex->getTraceAsString())); - echo(''); + \header("{$_SERVER['SERVER_PROTOCOL']} 599 Broken"); + \OC::crashLog($ex); + \OC::crashLog($ex2); } -} catch (Error $ex) { - \OC::$server->getLogger()->logException($ex, ['app' => 'index']); - OC_Response::setStatus(OC_Response::STATUS_INTERNAL_SERVER_ERROR); - OC_Template::printExceptionErrorPage($ex); } diff --git a/lib/base.php b/lib/base.php index 24346a921341..56c9d643473c 100644 --- a/lib/base.php +++ b/lib/base.php @@ -1000,6 +1000,59 @@ protected static function handleAuthHeaders() { } } } + + /** + * Log an ownCloud's crash. This means that ownCloud isn't usable and can't log through + * normal ownCloud's logger facilities. + * This crash log can't use the normal "owncloud.log" file because the ownCloud's logger + * requires additional context that can't be easily replicated. We'll use a different file. + * The crash log will create a "crash-Y-m-d.log" file in the ownCloud's data directory, unless + * the "crashdirectory" is set in the config.php file (make sure the "crashdirectory" exists and + * it's writeable for the web server). The filename will be reused for all the crashes that happen + * during the same day, and a new one will be used the next day. + * The crash file will be created only if needed. + * + * Note: This is mainly for internal purposes. You're encouraged to use the ownCloud's logger + * for anything you need to log. + */ + public static function crashLog(\Throwable $ex) { + $dataDir = self::$config->getValue('datadirectory', self::$SERVERROOT . '/data'); + $crashDir = self::$config->getValue('crashdirectory', $dataDir); + + $filename = "${crashDir}/crash-" . \date('Y-m-d') . '.log'; + + $date = \date('c'); + $currentEntryId = \uniqid(\md5($date), true); + $entry = [ + 'date' => $date, + 'parentId' => null, + 'id' => $currentEntryId, + 'class' => \get_class($ex), + 'message' => $ex->getMessage(), + 'stacktrace' => \array_map(function ($elem) { + unset($elem['args'], $elem['type']); + return $elem; + }, $ex->getTrace()), + ]; + \file_put_contents($filename, \json_encode($entry, JSON_PRETTY_PRINT) . PHP_EOL, FILE_APPEND | LOCK_EX); + + while (($ex = $ex->getPrevious()) !== null) { + $previousEntryId = $currentEntryId; + $currentEntryId = \uniqid(\md5($date), true); + $entry = [ + 'date' => $date, + 'parentId' => $previousEntryId, + 'id' => $currentEntryId, + 'class' => \get_class($ex), + 'message' => $ex->getMessage(), + 'stacktrace' => \array_map(function ($elem) { + unset($elem['args'], $elem['type']); + return $elem; + }, $ex->getTrace()), + ]; + \file_put_contents($filename, \json_encode($entry, JSON_PRETTY_PRINT) . PHP_EOL, FILE_APPEND | LOCK_EX); + } + } } OC::init(); diff --git a/public.php b/public.php index 29820fcf7e1c..5431a988f568 100644 --- a/public.php +++ b/public.php @@ -73,18 +73,20 @@ $baseuri = OC::$WEBROOT . '/public.php/' . $service . '/'; require_once OC_App::getAppPath($app) . '/' . $parts[1]; -} catch (Exception $ex) { - if ($ex instanceof \OC\ServiceUnavailableException) { - OC_Response::setStatus(OC_Response::STATUS_SERVICE_UNAVAILABLE); - } else { - OC_Response::setStatus(OC_Response::STATUS_INTERNAL_SERVER_ERROR); +} catch (\Throwable $ex) { + try { + if ($ex instanceof \OC\ServiceUnavailableException) { + OC_Response::setStatus(OC_Response::STATUS_SERVICE_UNAVAILABLE); + } else { + OC_Response::setStatus(OC_Response::STATUS_INTERNAL_SERVER_ERROR); + } + //show the user a detailed error page + \OC::$server->getLogger()->logException($ex, ['app' => 'public']); + OC_Template::printExceptionErrorPage($ex); + } catch (\Throwable $ex2) { + // log through the crashLog + \header("{$_SERVER['SERVER_PROTOCOL']} 599 Broken"); + \OC::crashLog($ex); + \OC::crashLog($ex2); } - //show the user a detailed error page - \OC::$server->getLogger()->logException($ex, ['app' => 'public']); - OC_Template::printExceptionErrorPage($ex); -} catch (Error $ex) { - //show the user a detailed error page - OC_Response::setStatus(OC_Response::STATUS_INTERNAL_SERVER_ERROR); - \OC::$server->getLogger()->logException($ex, ['app' => 'public']); - OC_Template::printExceptionErrorPage($ex); } diff --git a/remote.php b/remote.php index 87d67db8dd4a..967902855c1f 100644 --- a/remote.php +++ b/remote.php @@ -163,8 +163,13 @@ function resolveService($service) { } $baseuri = OC::$WEBROOT . '/remote.php/'.$service.'/'; require_once $file; -} catch (Exception $ex) { - handleException($ex); -} catch (Error $e) { - handleException($e); +} catch (\Throwable $ex) { + try { + handleException($ex); + } catch (\Throwable $ex2) { + // log through the crashLog + \header("{$_SERVER['SERVER_PROTOCOL']} 599 Broken"); + \OC::crashLog($ex); + \OC::crashLog($ex2); + } } diff --git a/status.php b/status.php index 93955d83c577..0f3a5c11a16f 100644 --- a/status.php +++ b/status.php @@ -48,7 +48,14 @@ \header('Content-Type: application/json'); echo \json_encode($values); } -} catch (Exception $ex) { - OC_Response::setStatus(OC_Response::STATUS_INTERNAL_SERVER_ERROR); - \OCP\Util::writeLog('remote', $ex->getMessage(), \OCP\Util::FATAL); +} catch (\Throwable $ex) { + try { + OC_Response::setStatus(OC_Response::STATUS_INTERNAL_SERVER_ERROR); + \OCP\Util::writeLog('remote', $ex->getMessage(), \OCP\Util::FATAL); + } catch (\Throwable $ex2) { + // log through the crashLog + \header("{$_SERVER['SERVER_PROTOCOL']} 599 Broken"); + \OC::crashLog($ex); + \OC::crashLog($ex2); + } } From 3674fa0c9b0ddb6c91996b137f435be78a1b24ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Thu, 28 Nov 2019 11:34:52 +0100 Subject: [PATCH 5/5] Add changelog item --- changelog/unreleased/36413 | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changelog/unreleased/36413 diff --git a/changelog/unreleased/36413 b/changelog/unreleased/36413 new file mode 100644 index 000000000000..a3aa30733ed7 --- /dev/null +++ b/changelog/unreleased/36413 @@ -0,0 +1,11 @@ +Bugfix: Set 599 HTTP code on error + +Previously, a hard crash, such as DB being down, was being reported with a stacktrace +and a 200 HTTP code. In order to homogenize the behaviour with all the endpoints, we've +changed the behaviour to return a 599 HTTP code and an empty content. + +In addition, we've included a new option in the config.php, "crashdirectory", which +defaults to the "datadirectory" set, where the crash logs will be created. Note that +normal errors will still be reported normally and will log into the owncloud.log file. + +https://github.com/owncloud/core/pull/36413 \ No newline at end of file