From 445924a2a23c4e95a91b5fc20a48e60c60b1914e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Sep 2016 12:25:37 -0700 Subject: [PATCH] (stable) Split setup check phases into "preflight" and "post-config" Summary: Ref T11589. This runs: - preflight checks (critical checks: PHP version stuff, extensions); - configuration; - normal checks. The PHP checks are split into critical ("bad version") and noncritical ("sub-optimal config"). I tidied up the extension checks slightly, we realistically depend on `cURL` nowadays. Test Plan: - Faked a preflight failure. - Hit preflight check. - Got expected error screen. - Loaded normal pages. - Hit a normal setup check. - Used DarkConsole "Startup" tab to verify that preflight checks take <1ms to run (we run them on every page without caching, at least for now, but they only do trivial checks like PHP versions). Reviewers: chad Reviewed By: chad Maniphest Tasks: T11589 Differential Revision: https://secure.phabricator.com/D16500 --- src/__phutil_library_map__.php | 2 + .../AphrontApplicationConfiguration.php | 9 + .../check/PhabricatorExtensionsSetupCheck.php | 6 +- .../check/PhabricatorPHPConfigSetupCheck.php | 197 +---------------- .../PhabricatorPHPPreflightSetupCheck.php | 201 ++++++++++++++++++ .../config/check/PhabricatorSetupCheck.php | 41 +++- .../PhabricatorConfigIssueListController.php | 2 +- .../PhabricatorConfigIssuePanelController.php | 2 +- .../PhabricatorConfigIssueViewController.php | 2 +- src/infrastructure/env/PhabricatorEnv.php | 9 +- 10 files changed, 267 insertions(+), 204 deletions(-) create mode 100644 src/applications/config/check/PhabricatorPHPPreflightSetupCheck.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9492d65df6..02b4dc5b92 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3000,6 +3000,7 @@ phutil_register_library_map(array( 'PhabricatorPHPASTApplication' => 'applications/phpast/application/PhabricatorPHPASTApplication.php', 'PhabricatorPHPConfigSetupCheck' => 'applications/config/check/PhabricatorPHPConfigSetupCheck.php', 'PhabricatorPHPMailerConfigOptions' => 'applications/config/option/PhabricatorPHPMailerConfigOptions.php', + 'PhabricatorPHPPreflightSetupCheck' => 'applications/config/check/PhabricatorPHPPreflightSetupCheck.php', 'PhabricatorPackagesApplication' => 'applications/packages/application/PhabricatorPackagesApplication.php', 'PhabricatorPackagesController' => 'applications/packages/controller/PhabricatorPackagesController.php', 'PhabricatorPackagesCreatePublisherCapability' => 'applications/packages/capability/PhabricatorPackagesCreatePublisherCapability.php', @@ -7858,6 +7859,7 @@ phutil_register_library_map(array( 'PhabricatorPHPASTApplication' => 'PhabricatorApplication', 'PhabricatorPHPConfigSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorPHPMailerConfigOptions' => 'PhabricatorApplicationConfigOptions', + 'PhabricatorPHPPreflightSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorPackagesApplication' => 'PhabricatorApplication', 'PhabricatorPackagesController' => 'PhabricatorController', 'PhabricatorPackagesCreatePublisherCapability' => 'PhabricatorPolicyCapability', diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index dc32faedec..7c5c571b64 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -69,6 +69,15 @@ abstract class AphrontApplicationConfiguration extends Phobject { // request object first. $write_guard = new AphrontWriteGuard('id'); + PhabricatorStartup::beginStartupPhase('preflight'); + + $response = PhabricatorSetupCheck::willPreflightRequest(); + if ($response) { + PhabricatorStartup::endOutputCapture(); + $sink->writeResponse($response); + return; + } + PhabricatorStartup::beginStartupPhase('env.init'); PhabricatorEnv::initializeWebEnvironment(); diff --git a/src/applications/config/check/PhabricatorExtensionsSetupCheck.php b/src/applications/config/check/PhabricatorExtensionsSetupCheck.php index 0840e545e4..973c80629b 100644 --- a/src/applications/config/check/PhabricatorExtensionsSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtensionsSetupCheck.php @@ -12,7 +12,6 @@ final class PhabricatorExtensionsSetupCheck extends PhabricatorSetupCheck { protected function executeChecks() { // TODO: Make 'mbstring' and 'iconv' soft requirements. - // TODO: Make 'curl' a soft requirement. $required = array( 'hash', @@ -22,9 +21,8 @@ final class PhabricatorExtensionsSetupCheck extends PhabricatorSetupCheck { 'iconv', 'ctype', - // There is a chance we might not need this, but some configurations (like - // OAuth or Amazon SES) will require it. Just mark it 'required' since - // it's widely available and relatively core. + // There is a tiny chance we might not need this, but a significant + // number of applications require it and it's widely available. 'curl', ); diff --git a/src/applications/config/check/PhabricatorPHPConfigSetupCheck.php b/src/applications/config/check/PhabricatorPHPConfigSetupCheck.php index 86e885614a..f286b46f9c 100644 --- a/src/applications/config/check/PhabricatorPHPConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorPHPConfigSetupCheck.php @@ -1,202 +1,17 @@ =')) { - $message = pht( - 'This version of Phabricator does not support PHP 7. You '. - 'are running PHP %s.', - phpversion()); - - $this->newIssue('php.version7') - ->setIsFatal(true) - ->setName(pht('PHP 7 Not Supported')) - ->setMessage($message) - ->addLink( - 'https://phurl.io/u/php7', - pht('Phabricator PHP 7 Compatibility Information')); - - return; - } - - $safe_mode = ini_get('safe_mode'); - if ($safe_mode) { - $message = pht( - "You have '%s' enabled in your PHP configuration, but Phabricator ". - "will not run in safe mode. Safe mode has been deprecated in PHP 5.3 ". - "and removed in PHP 5.4.\n\nDisable safe mode to continue.", - 'safe_mode'); - - $this->newIssue('php.safe_mode') - ->setIsFatal(true) - ->setName(pht('Disable PHP %s', 'safe_mode')) - ->setMessage($message) - ->addPHPConfig('safe_mode'); - return; - } - - // Check for `disable_functions` or `disable_classes`. Although it's - // possible to disable a bunch of functions (say, `array_change_key_case()`) - // and classes and still have Phabricator work fine, it's unreasonably - // difficult for us to be sure we'll even survive setup if these options - // are enabled. Phabricator needs access to the most dangerous functions, - // so there is no reasonable configuration value here which actually - // provides a benefit while guaranteeing Phabricator will run properly. - - $disable_options = array('disable_functions', 'disable_classes'); - foreach ($disable_options as $disable_option) { - $disable_value = ini_get($disable_option); - if ($disable_value) { - - // By default Debian installs the pcntl extension but disables all of - // its functions using configuration. Whitelist disabling these - // functions so that Debian PHP works out of the box (we do not need to - // call these functions from the web UI). This is pretty ridiculous but - // it's not the users' fault and they haven't done anything crazy to - // get here, so don't make them pay for Debian's unusual choices. - // See: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=605571 - $fatal = true; - if ($disable_option == 'disable_functions') { - $functions = preg_split('/[, ]+/', $disable_value); - $functions = array_filter($functions); - foreach ($functions as $k => $function) { - if (preg_match('/^pcntl_/', $function)) { - unset($functions[$k]); - } - } - if (!$functions) { - $fatal = false; - } - } - - if ($fatal) { - $message = pht( - "You have '%s' enabled in your PHP configuration.\n\n". - "This option is not compatible with Phabricator. Remove ". - "'%s' from your configuration to continue.", - $disable_option, - $disable_option); - - $this->newIssue('php.'.$disable_option) - ->setIsFatal(true) - ->setName(pht('Remove PHP %s', $disable_option)) - ->setMessage($message) - ->addPHPConfig($disable_option); - } - } - } - - $overload_option = 'mbstring.func_overload'; - $func_overload = ini_get($overload_option); - if ($func_overload) { - $message = pht( - "You have '%s' enabled in your PHP configuration.\n\n". - "This option is not compatible with Phabricator. Disable ". - "'%s' in your PHP configuration to continue.", - $overload_option, - $overload_option); - - $this->newIssue('php'.$overload_option) - ->setIsFatal(true) - ->setName(pht('Disable PHP %s', $overload_option)) - ->setMessage($message) - ->addPHPConfig($overload_option); - } - - $open_basedir = ini_get('open_basedir'); - if ($open_basedir) { - - // 'open_basedir' restricts which files we're allowed to access with - // file operations. This might be okay -- we don't need to write to - // arbitrary places in the filesystem -- but we need to access certain - // resources. This setting is unlikely to be providing any real measure - // of security so warn even if things look OK. - - $failures = array(); - - try { - $open_libphutil = class_exists('Future'); - } catch (Exception $ex) { - $failures[] = $ex->getMessage(); - } - - try { - $open_arcanist = class_exists('ArcanistDiffParser'); - } catch (Exception $ex) { - $failures[] = $ex->getMessage(); - } - - $open_urandom = false; - try { - Filesystem::readRandomBytes(1); - $open_urandom = true; - } catch (FilesystemException $ex) { - $failures[] = $ex->getMessage(); - } - - try { - $tmp = new TempFile(); - file_put_contents($tmp, '.'); - $open_tmp = @fopen((string)$tmp, 'r'); - if (!$open_tmp) { - $failures[] = pht( - "Unable to read temporary file '%s'.", - (string)$tmp); - } - } catch (Exception $ex) { - $message = $ex->getMessage(); - $dir = sys_get_temp_dir(); - $failures[] = pht( - "Unable to open temp files from '%s': %s", - $dir, - $message); - } - - $issue = $this->newIssue('php.open_basedir') - ->setName(pht('Disable PHP %s', 'open_basedir')) - ->addPHPConfig('open_basedir'); - - if ($failures) { - $message = pht( - "Your server is configured with '%s', which prevents Phabricator ". - "from opening files it requires access to.\n\n". - "Disable this setting to continue.\n\nFailures:\n\n%s", - 'open_basedir', - implode("\n\n", $failures)); - - $issue - ->setIsFatal(true) - ->setMessage($message); - - return; - } else { - $summary = pht( - "You have '%s' configured in your PHP settings, which ". - "may cause some features to fail.", - 'open_basedir'); - - $message = pht( - "You have '%s' configured in your PHP settings. Although this ". - "setting appears permissive enough that Phabricator will work ". - "properly, you may still run into problems because of it.\n\n". - "Consider disabling '%s'.", - 'open_basedir', - 'open_basedir'); - - $issue - ->setSummary($summary) - ->setMessage($message); - } - } if (empty($_SERVER['REMOTE_ADDR'])) { $doc_href = PhabricatorEnv::getDocLink('Configuring a Preamble Script'); @@ -245,5 +60,7 @@ final class PhabricatorPHPConfigSetupCheck extends PhabricatorSetupCheck { ->setMessage($message) ->addPHPConfig('always_populate_raw_post_data'); } + } + } diff --git a/src/applications/config/check/PhabricatorPHPPreflightSetupCheck.php b/src/applications/config/check/PhabricatorPHPPreflightSetupCheck.php new file mode 100644 index 0000000000..65a582b29c --- /dev/null +++ b/src/applications/config/check/PhabricatorPHPPreflightSetupCheck.php @@ -0,0 +1,201 @@ +=')) { + $message = pht( + 'This version of Phabricator does not support PHP 7. You '. + 'are running PHP %s.', + phpversion()); + + $this->newIssue('php.version7') + ->setIsFatal(true) + ->setName(pht('PHP 7 Not Supported')) + ->setMessage($message) + ->addLink( + 'https://phurl.io/u/php7', + pht('Phabricator PHP 7 Compatibility Information')); + + return; + } + + $safe_mode = ini_get('safe_mode'); + if ($safe_mode) { + $message = pht( + "You have '%s' enabled in your PHP configuration, but Phabricator ". + "will not run in safe mode. Safe mode has been deprecated in PHP 5.3 ". + "and removed in PHP 5.4.\n\nDisable safe mode to continue.", + 'safe_mode'); + + $this->newIssue('php.safe_mode') + ->setIsFatal(true) + ->setName(pht('Disable PHP %s', 'safe_mode')) + ->setMessage($message) + ->addPHPConfig('safe_mode'); + return; + } + + // Check for `disable_functions` or `disable_classes`. Although it's + // possible to disable a bunch of functions (say, `array_change_key_case()`) + // and classes and still have Phabricator work fine, it's unreasonably + // difficult for us to be sure we'll even survive setup if these options + // are enabled. Phabricator needs access to the most dangerous functions, + // so there is no reasonable configuration value here which actually + // provides a benefit while guaranteeing Phabricator will run properly. + + $disable_options = array('disable_functions', 'disable_classes'); + foreach ($disable_options as $disable_option) { + $disable_value = ini_get($disable_option); + if ($disable_value) { + + // By default Debian installs the pcntl extension but disables all of + // its functions using configuration. Whitelist disabling these + // functions so that Debian PHP works out of the box (we do not need to + // call these functions from the web UI). This is pretty ridiculous but + // it's not the users' fault and they haven't done anything crazy to + // get here, so don't make them pay for Debian's unusual choices. + // See: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=605571 + $fatal = true; + if ($disable_option == 'disable_functions') { + $functions = preg_split('/[, ]+/', $disable_value); + $functions = array_filter($functions); + foreach ($functions as $k => $function) { + if (preg_match('/^pcntl_/', $function)) { + unset($functions[$k]); + } + } + if (!$functions) { + $fatal = false; + } + } + + if ($fatal) { + $message = pht( + "You have '%s' enabled in your PHP configuration.\n\n". + "This option is not compatible with Phabricator. Remove ". + "'%s' from your configuration to continue.", + $disable_option, + $disable_option); + + $this->newIssue('php.'.$disable_option) + ->setIsFatal(true) + ->setName(pht('Remove PHP %s', $disable_option)) + ->setMessage($message) + ->addPHPConfig($disable_option); + } + } + } + + $overload_option = 'mbstring.func_overload'; + $func_overload = ini_get($overload_option); + if ($func_overload) { + $message = pht( + "You have '%s' enabled in your PHP configuration.\n\n". + "This option is not compatible with Phabricator. Disable ". + "'%s' in your PHP configuration to continue.", + $overload_option, + $overload_option); + + $this->newIssue('php'.$overload_option) + ->setIsFatal(true) + ->setName(pht('Disable PHP %s', $overload_option)) + ->setMessage($message) + ->addPHPConfig($overload_option); + } + + $open_basedir = ini_get('open_basedir'); + if ($open_basedir) { + + // 'open_basedir' restricts which files we're allowed to access with + // file operations. This might be okay -- we don't need to write to + // arbitrary places in the filesystem -- but we need to access certain + // resources. This setting is unlikely to be providing any real measure + // of security so warn even if things look OK. + + $failures = array(); + + try { + $open_libphutil = class_exists('Future'); + } catch (Exception $ex) { + $failures[] = $ex->getMessage(); + } + + try { + $open_arcanist = class_exists('ArcanistDiffParser'); + } catch (Exception $ex) { + $failures[] = $ex->getMessage(); + } + + $open_urandom = false; + try { + Filesystem::readRandomBytes(1); + $open_urandom = true; + } catch (FilesystemException $ex) { + $failures[] = $ex->getMessage(); + } + + try { + $tmp = new TempFile(); + file_put_contents($tmp, '.'); + $open_tmp = @fopen((string)$tmp, 'r'); + if (!$open_tmp) { + $failures[] = pht( + "Unable to read temporary file '%s'.", + (string)$tmp); + } + } catch (Exception $ex) { + $message = $ex->getMessage(); + $dir = sys_get_temp_dir(); + $failures[] = pht( + "Unable to open temp files from '%s': %s", + $dir, + $message); + } + + $issue = $this->newIssue('php.open_basedir') + ->setName(pht('Disable PHP %s', 'open_basedir')) + ->addPHPConfig('open_basedir'); + + if ($failures) { + $message = pht( + "Your server is configured with '%s', which prevents Phabricator ". + "from opening files it requires access to.\n\n". + "Disable this setting to continue.\n\nFailures:\n\n%s", + 'open_basedir', + implode("\n\n", $failures)); + + $issue + ->setIsFatal(true) + ->setMessage($message); + + return; + } else { + $summary = pht( + "You have '%s' configured in your PHP settings, which ". + "may cause some features to fail.", + 'open_basedir'); + + $message = pht( + "You have '%s' configured in your PHP settings. Although this ". + "setting appears permissive enough that Phabricator will work ". + "properly, you may still run into problems because of it.\n\n". + "Consider disabling '%s'.", + 'open_basedir', + 'open_basedir'); + + $issue + ->setSummary($summary) + ->setMessage($message); + } + } + } +} diff --git a/src/applications/config/check/PhabricatorSetupCheck.php b/src/applications/config/check/PhabricatorSetupCheck.php index 5970b0ca81..b1f79bdd47 100644 --- a/src/applications/config/check/PhabricatorSetupCheck.php +++ b/src/applications/config/check/PhabricatorSetupCheck.php @@ -129,16 +129,39 @@ abstract class PhabricatorSetupCheck extends Phobject { )); } + final public static function willPreflightRequest() { + $checks = self::loadAllChecks(); + + foreach ($checks as $check) { + if (!$check->isPreflightCheck()) { + continue; + } + + $check->runSetupChecks(); + + foreach ($check->getIssues() as $key => $issue) { + return self::newIssueResponse($issue); + } + } + + return null; + } + + private static function newIssueResponse(PhabricatorSetupIssue $issue) { + $view = id(new PhabricatorSetupIssueView()) + ->setIssue($issue); + + return id(new PhabricatorConfigResponse()) + ->setView($view); + } + final public static function willProcessRequest() { $issue_keys = self::getOpenSetupIssueKeys(); if ($issue_keys === null) { - $issues = self::runAllChecks(); + $issues = self::runNormalChecks(); foreach ($issues as $issue) { if ($issue->getIsFatal()) { - $view = id(new PhabricatorSetupIssueView()) - ->setIssue($issue); - return id(new PhabricatorConfigResponse()) - ->setView($view); + return self::newIssueResponse($issue); } } $issue_keys = self::getUnignoredIssueKeys($issues); @@ -176,9 +199,15 @@ abstract class PhabricatorSetupCheck extends Phobject { ->execute(); } - final public static function runAllChecks() { + final public static function runNormalChecks() { $checks = self::loadAllChecks(); + foreach ($checks as $key => $check) { + if ($check->isPreflightCheck()) { + unset($checks[$key]); + } + } + $issues = array(); foreach ($checks as $check) { $check->runSetupChecks(); diff --git a/src/applications/config/controller/PhabricatorConfigIssueListController.php b/src/applications/config/controller/PhabricatorConfigIssueListController.php index 7f0b19af69..a5553a34bb 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueListController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueListController.php @@ -9,7 +9,7 @@ final class PhabricatorConfigIssueListController $nav = $this->buildSideNavView(); $nav->selectFilter('issue/'); - $issues = PhabricatorSetupCheck::runAllChecks(); + $issues = PhabricatorSetupCheck::runNormalChecks(); PhabricatorSetupCheck::setOpenSetupIssueKeys( PhabricatorSetupCheck::getUnignoredIssueKeys($issues), $update_database = true); diff --git a/src/applications/config/controller/PhabricatorConfigIssuePanelController.php b/src/applications/config/controller/PhabricatorConfigIssuePanelController.php index 16541d184b..3dd910f693 100644 --- a/src/applications/config/controller/PhabricatorConfigIssuePanelController.php +++ b/src/applications/config/controller/PhabricatorConfigIssuePanelController.php @@ -6,7 +6,7 @@ final class PhabricatorConfigIssuePanelController public function handleRequest(AphrontRequest $request) { $viewer = $request->getViewer(); $open_items = PhabricatorSetupCheck::getOpenSetupIssueKeys(); - $issues = PhabricatorSetupCheck::runAllChecks(); + $issues = PhabricatorSetupCheck::runNormalChecks(); PhabricatorSetupCheck::setOpenSetupIssueKeys( PhabricatorSetupCheck::getUnignoredIssueKeys($issues), $update_database = true); diff --git a/src/applications/config/controller/PhabricatorConfigIssueViewController.php b/src/applications/config/controller/PhabricatorConfigIssueViewController.php index 4700e6315e..57d7b75137 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueViewController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueViewController.php @@ -7,7 +7,7 @@ final class PhabricatorConfigIssueViewController $viewer = $request->getViewer(); $issue_key = $request->getURIData('key'); - $issues = PhabricatorSetupCheck::runAllChecks(); + $issues = PhabricatorSetupCheck::runNormalChecks(); PhabricatorSetupCheck::setOpenSetupIssueKeys( PhabricatorSetupCheck::getUnignoredIssueKeys($issues), $update_database = true); diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index d66e022465..221d553d2b 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -315,6 +315,14 @@ final class PhabricatorEnv extends Phobject { return self::$cache[$key]; } + if (!self::$sourceStack) { + throw new Exception( + pht( + 'Trying to read configuration "%s" before configuration has been '. + 'initialized.', + $key)); + } + $result = self::$sourceStack->getKeys(array($key)); if (array_key_exists($key, $result)) { self::$cache[$key] = $result[$key]; @@ -327,7 +335,6 @@ final class PhabricatorEnv extends Phobject { } } - /** * Get the current configuration setting for a given key. If the key * does not exist, return a default value instead of throwing. This is