From ffdc082852fe4db562f7f7b750c8146178d7c034 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 8 Dec 2016 15:21:20 -0800 Subject: [PATCH] Add a wide range of HTTP-request-based setup checks Summary: Ref T11553. With some regularity, users make various configuration mistakes which we can detect by making a request to ourselves. I use a magical header to make this request because we want to test everything else (parameters, path). - Fixes T4854, probably. Tries to detect mod_pagespeed by looking for a header. This is a documentation-based "fix", I didn't actually install mod_pagespeed or formally test this. - Fixes T6866. We now test for parameters (e.g., user somehow lost "QSA"). - Ref T6709. We now test that stuff is decoded exactly once (e.g., user somehow lost "B"). - Fixes T4921. We now test that Authorization survives the request. - Fixes T2226. Adds a setup check to determine whether gzip is enabled on the web server, and attempts to enable it at the PHP level. - Fixes ` 'applications/people/typeahead/PhabricatorViewerDatasource.php', 'PhabricatorWatcherHasObjectEdgeType' => 'applications/transactions/edges/PhabricatorWatcherHasObjectEdgeType.php', 'PhabricatorWebContentSource' => 'infrastructure/contentsource/PhabricatorWebContentSource.php', + 'PhabricatorWebServerSetupCheck' => 'applications/config/check/PhabricatorWebServerSetupCheck.php', 'PhabricatorWeekStartDaySetting' => 'applications/settings/setting/PhabricatorWeekStartDaySetting.php', 'PhabricatorWordPressAuthProvider' => 'applications/auth/provider/PhabricatorWordPressAuthProvider.php', 'PhabricatorWorker' => 'infrastructure/daemon/workers/PhabricatorWorker.php', @@ -9216,6 +9217,7 @@ phutil_register_library_map(array( 'PhabricatorViewerDatasource' => 'PhabricatorTypeaheadDatasource', 'PhabricatorWatcherHasObjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorWebContentSource' => 'PhabricatorContentSource', + 'PhabricatorWebServerSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorWeekStartDaySetting' => 'PhabricatorSelectSetting', 'PhabricatorWordPressAuthProvider' => 'PhabricatorOAuth2AuthProvider', 'PhabricatorWorker' => 'Phobject', diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index bbfcaf0b46..aee2c63032 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -59,6 +59,11 @@ abstract class AphrontApplicationConfiguration extends Phobject { * @phutil-external-symbol class PhabricatorStartup */ public static function runHTTPRequest(AphrontHTTPSink $sink) { + if (isset($_SERVER['HTTP_X_PHABRICATOR_SELFCHECK'])) { + $response = self::newSelfCheckResponse(); + return self::writeResponse($sink, $response); + } + PhabricatorStartup::beginStartupPhase('multimeter'); $multimeter = MultimeterControl::newInstance(); $multimeter->setEventContext(''); @@ -690,5 +695,36 @@ abstract class AphrontApplicationConfiguration extends Phobject { throw $ex; } + private static function newSelfCheckResponse() { + $path = idx($_REQUEST, '__path__', ''); + $query = idx($_SERVER, 'QUERY_STRING', ''); + + $pairs = id(new PhutilQueryStringParser()) + ->parseQueryStringToPairList($query); + + $params = array(); + foreach ($pairs as $v) { + $params[] = array( + 'name' => $v[0], + 'value' => $v[1], + ); + } + + $result = array( + 'path' => $path, + 'params' => $params, + 'user' => idx($_SERVER, 'PHP_AUTH_USER'), + 'pass' => idx($_SERVER, 'PHP_AUTH_PW'), + + // This just makes sure that the response compresses well, so reasonable + // algorithms should want to gzip or deflate it. + 'filler' => str_repeat('Q', 1024 * 16), + ); + + + return id(new AphrontJSONResponse()) + ->setAddJSONShield(false) + ->setContent($result); + } } diff --git a/src/applications/config/check/PhabricatorWebServerSetupCheck.php b/src/applications/config/check/PhabricatorWebServerSetupCheck.php new file mode 100644 index 0000000000..346dd2de1e --- /dev/null +++ b/src/applications/config/check/PhabricatorWebServerSetupCheck.php @@ -0,0 +1,224 @@ +newIssue('webserver.pagespeed') + ->setName(pht('Disable Pagespeed')) + ->setSummary(pht('Pagespeed is enabled, but should be disabled.')) + ->setMessage( + pht( + 'Phabricator received an "X-Mod-Pagespeed" or "X-Page-Speed" '. + 'HTTP header on this request, which indicates that you have '. + 'enabled "mod_pagespeed" on this server. This module is not '. + 'compatible with Phabricator. You should disable it.')); + } + + $base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri'); + if (!strlen($base_uri)) { + // If `phabricator.base-uri` is not set then we can't really do + // anything. + return; + } + + $expect_user = 'alincoln'; + $expect_pass = 'hunter2'; + + $send_path = '/test-%252A/'; + $expect_path = '/test-%2A/'; + + $expect_key = 'duck-sound'; + $expect_value = 'quack'; + + $base_uri = id(new PhutilURI($base_uri)) + ->setPath($send_path) + ->setQueryParam($expect_key, $expect_value); + + $future = id(new HTTPSFuture($base_uri)) + ->addHeader('X-Phabricator-SelfCheck', 1) + ->addHeader('Accept-Encoding', 'gzip') + ->setHTTPBasicAuthCredentials( + $expect_user, + new PhutilOpaqueEnvelope($expect_pass)) + ->setTimeout(5); + + try { + list($body, $headers) = $future->resolvex(); + } catch (Exception $ex) { + // If this fails for whatever reason, just ignore it. Hopefully, the + // error is obvious and the user can correct it on their own, but we + // can't do much to offer diagnostic advice. + return; + } + + if (BaseHTTPFuture::getHeader($headers, 'Content-Encoding') != 'gzip') { + $message = pht( + 'Phabricator sent itself a request with "Accept-Encoding: gzip", '. + 'but received an uncompressed response.'. + "\n\n". + 'This may indicate that your webserver is not configured to '. + 'compress responses. If so, you should enable compression. '. + 'Compression can dramatically improve performance, especially '. + 'for clients with less bandwidth.'); + + $this->newIssue('webserver.gzip') + ->setName(pht('GZip Compression May Not Be Enabled')) + ->setSummary(pht('Your webserver may have compression disabled.')) + ->setMessage($message); + } else { + if (function_exists('gzdecode')) { + $body = gzdecode($body); + } else { + $body = null; + } + if (!$body) { + // For now, just bail if we can't decode the response. + // This might need to use the stronger magic in "AphrontRequestStream" + // to decode more reliably. + return; + } + } + + $structure = null; + $caught = null; + $extra_whitespace = ($body !== trim($body)); + + if (!$extra_whitespace) { + try { + $structure = phutil_json_decode($body); + } catch (Exception $ex) { + $caught = $ex; + } + } + + if (!$structure) { + if ($extra_whitespace) { + $message = pht( + 'Phabricator sent itself a test request and expected to get a bare '. + 'JSON response back, but the response had extra whitespace at '. + 'the beginning or end.'. + "\n\n". + 'This usually means you have edited a file and left whitespace '. + 'characters before the opening %s tag, or after a closing %s tag. '. + 'Remove any leading whitespace, and prefer to omit closing tags.', + phutil_tag('tt', array(), '')); + } else { + $short = id(new PhutilUTF8StringTruncator()) + ->setMaximumGlyphs(1024) + ->truncateString($body); + + $message = pht( + 'Phabricator sent itself a test request with the '. + '"X-Phabricator-SelfCheck" header and expected to get a valid JSON '. + 'response back. Instead, the response begins:'. + "\n\n". + '%s'. + "\n\n". + 'Something is misconfigured or otherwise mangling responses.', + phutil_tag('pre', array(), $short)); + } + + $this->newIssue('webserver.mangle') + ->setName(pht('Mangled Webserver Response')) + ->setSummary(pht('Your webserver produced an unexpected response.')) + ->setMessage($message); + + // We can't run the other checks if we could not decode the response. + return; + } + + $actual_user = idx($structure, 'user'); + $actual_pass = idx($structure, 'pass'); + if (($expect_user != $actual_user) || ($actual_pass != $expect_pass)) { + $message = pht( + 'Phabricator sent itself a test request with an "Authorization" HTTP '. + 'header, and expected those credentials to be transmitted. However, '. + 'they were absent or incorrect when received. Phabricator sent '. + 'username "%s" with password "%s"; received username "%s" and '. + 'password "%s".'. + "\n\n". + 'Your webserver may not be configured to forward HTTP basic '. + 'authentication. If you plan to use basic authentication (for '. + 'example, to access repositories) you should reconfigure it.', + $expect_user, + $expect_pass, + $actual_user, + $actual_pass); + + $this->newIssue('webserver.basic-auth') + ->setName(pht('HTTP Basic Auth Not Configured')) + ->setSummary(pht('Your webserver is not forwarding credentials.')) + ->setMessage($message); + } + + $actual_path = idx($structure, 'path'); + if ($expect_path != $actual_path) { + $message = pht( + 'Phabricator sent itself a test request with an unusual path, to '. + 'test if your webserver is rewriting paths correctly. The path was '. + 'not transmitted correctly.'. + "\n\n". + 'Phabricator sent a request to path "%s", and expected the webserver '. + 'to decode and rewrite that path so that it received a request for '. + '"%s". However, it received a request for "%s" instead.'. + "\n\n". + 'Verify that your rewrite rules are configured correctly, following '. + 'the instructions in the documentation. If path encoding is not '. + 'working properly you will be unable to access files with unusual '. + 'names in repositories, among other issues.'. + "\n\n". + '(This problem can be caused by a missing "B" in your RewriteRule.)', + $send_path, + $expect_path, + $actual_path); + + $this->newIssue('webserver.rewrites') + ->setName(pht('HTTP Path Rewriting Incorrect')) + ->setSummary(pht('Your webserver is rewriting paths improperly.')) + ->setMessage($message); + } + + $actual_key = pht(''); + $actual_value = pht(''); + foreach (idx($structure, 'params', array()) as $pair) { + if (idx($pair, 'name') == $expect_key) { + $actual_key = idx($pair, 'name'); + $actual_value = idx($pair, 'value'); + break; + } + } + + if (($expect_key !== $actual_key) || ($expect_value !== $actual_value)) { + $message = pht( + 'Phabricator sent itself a test request with an HTTP GET parameter, '. + 'but the parameter was not transmitted. Sent "%s" with value "%s", '. + 'got "%s" with value "%s".'. + "\n\n". + 'Your webserver is configured incorrectly and large parts of '. + 'Phabricator will not work until this issue is corrected.'. + "\n\n". + '(This problem can be caused by a missing "QSA" in your RewriteRule.)', + $expect_key, + $expect_value, + $actual_key, + $actual_value); + + $this->newIssue('webserver.parameters') + ->setName(pht('HTTP Parameters Not Transmitting')) + ->setSummary( + pht('Your webserver is not handling GET parameters properly.')) + ->setMessage($message); + } + + } + +} diff --git a/support/PhabricatorStartup.php b/support/PhabricatorStartup.php index 0a27a616e3..26c6b26a9c 100644 --- a/support/PhabricatorStartup.php +++ b/support/PhabricatorStartup.php @@ -395,6 +395,11 @@ final class PhabricatorStartup { if (function_exists('libxml_disable_entity_loader')) { libxml_disable_entity_loader(true); } + + // Enable automatic compression here. Webservers sometimes do this for + // us, but we now detect the absence of compression and warn users about + // it so try to cover our bases more thoroughly. + ini_set('zlib.output_compression', 1); }