Skip to content

Commit ffdc082

Browse files
author
epriestley
committedDec 8, 2016
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 `<space space newline newline space><?php` in `preamble.php`. Test Plan: Tested all of these setup warnings, although mostly by faking them. Reviewers: joshuaspence, chad Reviewed By: chad Subscribers: Korvin Maniphest Tasks: T4854, T4921, T6709, T6866, T11553, T2226 Differential Revision: https://secure.phabricator.com/D12622
1 parent be4f66a commit ffdc082

File tree

4 files changed

+267
-0
lines changed

4 files changed

+267
-0
lines changed
 

‎src/__phutil_library_map__.php

+2
Original file line numberDiff line numberDiff line change
@@ -3994,6 +3994,7 @@
39943994
'PhabricatorViewerDatasource' => 'applications/people/typeahead/PhabricatorViewerDatasource.php',
39953995
'PhabricatorWatcherHasObjectEdgeType' => 'applications/transactions/edges/PhabricatorWatcherHasObjectEdgeType.php',
39963996
'PhabricatorWebContentSource' => 'infrastructure/contentsource/PhabricatorWebContentSource.php',
3997+
'PhabricatorWebServerSetupCheck' => 'applications/config/check/PhabricatorWebServerSetupCheck.php',
39973998
'PhabricatorWeekStartDaySetting' => 'applications/settings/setting/PhabricatorWeekStartDaySetting.php',
39983999
'PhabricatorWordPressAuthProvider' => 'applications/auth/provider/PhabricatorWordPressAuthProvider.php',
39994000
'PhabricatorWorker' => 'infrastructure/daemon/workers/PhabricatorWorker.php',
@@ -9216,6 +9217,7 @@
92169217
'PhabricatorViewerDatasource' => 'PhabricatorTypeaheadDatasource',
92179218
'PhabricatorWatcherHasObjectEdgeType' => 'PhabricatorEdgeType',
92189219
'PhabricatorWebContentSource' => 'PhabricatorContentSource',
9220+
'PhabricatorWebServerSetupCheck' => 'PhabricatorSetupCheck',
92199221
'PhabricatorWeekStartDaySetting' => 'PhabricatorSelectSetting',
92209222
'PhabricatorWordPressAuthProvider' => 'PhabricatorOAuth2AuthProvider',
92219223
'PhabricatorWorker' => 'Phobject',

‎src/aphront/configuration/AphrontApplicationConfiguration.php

+36
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,11 @@ public function willBuildRequest() {}
5959
* @phutil-external-symbol class PhabricatorStartup
6060
*/
6161
public static function runHTTPRequest(AphrontHTTPSink $sink) {
62+
if (isset($_SERVER['HTTP_X_PHABRICATOR_SELFCHECK'])) {
63+
$response = self::newSelfCheckResponse();
64+
return self::writeResponse($sink, $response);
65+
}
66+
6267
PhabricatorStartup::beginStartupPhase('multimeter');
6368
$multimeter = MultimeterControl::newInstance();
6469
$multimeter->setEventContext('<http-init>');
@@ -690,5 +695,36 @@ private function handleException(Exception $ex) {
690695
throw $ex;
691696
}
692697

698+
private static function newSelfCheckResponse() {
699+
$path = idx($_REQUEST, '__path__', '');
700+
$query = idx($_SERVER, 'QUERY_STRING', '');
701+
702+
$pairs = id(new PhutilQueryStringParser())
703+
->parseQueryStringToPairList($query);
704+
705+
$params = array();
706+
foreach ($pairs as $v) {
707+
$params[] = array(
708+
'name' => $v[0],
709+
'value' => $v[1],
710+
);
711+
}
712+
713+
$result = array(
714+
'path' => $path,
715+
'params' => $params,
716+
'user' => idx($_SERVER, 'PHP_AUTH_USER'),
717+
'pass' => idx($_SERVER, 'PHP_AUTH_PW'),
718+
719+
// This just makes sure that the response compresses well, so reasonable
720+
// algorithms should want to gzip or deflate it.
721+
'filler' => str_repeat('Q', 1024 * 16),
722+
);
723+
724+
725+
return id(new AphrontJSONResponse())
726+
->setAddJSONShield(false)
727+
->setContent($result);
728+
}
693729

694730
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,224 @@
1+
<?php
2+
3+
final class PhabricatorWebServerSetupCheck extends PhabricatorSetupCheck {
4+
5+
public function getDefaultGroup() {
6+
return self::GROUP_OTHER;
7+
}
8+
9+
protected function executeChecks() {
10+
// The documentation says these headers exist, but it's not clear if they
11+
// are entirely reliable in practice.
12+
if (isset($_SERVER['HTTP_X_MOD_PAGESPEED']) ||
13+
isset($_SERVER['HTTP_X_PAGE_SPEED'])) {
14+
$this->newIssue('webserver.pagespeed')
15+
->setName(pht('Disable Pagespeed'))
16+
->setSummary(pht('Pagespeed is enabled, but should be disabled.'))
17+
->setMessage(
18+
pht(
19+
'Phabricator received an "X-Mod-Pagespeed" or "X-Page-Speed" '.
20+
'HTTP header on this request, which indicates that you have '.
21+
'enabled "mod_pagespeed" on this server. This module is not '.
22+
'compatible with Phabricator. You should disable it.'));
23+
}
24+
25+
$base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
26+
if (!strlen($base_uri)) {
27+
// If `phabricator.base-uri` is not set then we can't really do
28+
// anything.
29+
return;
30+
}
31+
32+
$expect_user = 'alincoln';
33+
$expect_pass = 'hunter2';
34+
35+
$send_path = '/test-%252A/';
36+
$expect_path = '/test-%2A/';
37+
38+
$expect_key = 'duck-sound';
39+
$expect_value = 'quack';
40+
41+
$base_uri = id(new PhutilURI($base_uri))
42+
->setPath($send_path)
43+
->setQueryParam($expect_key, $expect_value);
44+
45+
$future = id(new HTTPSFuture($base_uri))
46+
->addHeader('X-Phabricator-SelfCheck', 1)
47+
->addHeader('Accept-Encoding', 'gzip')
48+
->setHTTPBasicAuthCredentials(
49+
$expect_user,
50+
new PhutilOpaqueEnvelope($expect_pass))
51+
->setTimeout(5);
52+
53+
try {
54+
list($body, $headers) = $future->resolvex();
55+
} catch (Exception $ex) {
56+
// If this fails for whatever reason, just ignore it. Hopefully, the
57+
// error is obvious and the user can correct it on their own, but we
58+
// can't do much to offer diagnostic advice.
59+
return;
60+
}
61+
62+
if (BaseHTTPFuture::getHeader($headers, 'Content-Encoding') != 'gzip') {
63+
$message = pht(
64+
'Phabricator sent itself a request with "Accept-Encoding: gzip", '.
65+
'but received an uncompressed response.'.
66+
"\n\n".
67+
'This may indicate that your webserver is not configured to '.
68+
'compress responses. If so, you should enable compression. '.
69+
'Compression can dramatically improve performance, especially '.
70+
'for clients with less bandwidth.');
71+
72+
$this->newIssue('webserver.gzip')
73+
->setName(pht('GZip Compression May Not Be Enabled'))
74+
->setSummary(pht('Your webserver may have compression disabled.'))
75+
->setMessage($message);
76+
} else {
77+
if (function_exists('gzdecode')) {
78+
$body = gzdecode($body);
79+
} else {
80+
$body = null;
81+
}
82+
if (!$body) {
83+
// For now, just bail if we can't decode the response.
84+
// This might need to use the stronger magic in "AphrontRequestStream"
85+
// to decode more reliably.
86+
return;
87+
}
88+
}
89+
90+
$structure = null;
91+
$caught = null;
92+
$extra_whitespace = ($body !== trim($body));
93+
94+
if (!$extra_whitespace) {
95+
try {
96+
$structure = phutil_json_decode($body);
97+
} catch (Exception $ex) {
98+
$caught = $ex;
99+
}
100+
}
101+
102+
if (!$structure) {
103+
if ($extra_whitespace) {
104+
$message = pht(
105+
'Phabricator sent itself a test request and expected to get a bare '.
106+
'JSON response back, but the response had extra whitespace at '.
107+
'the beginning or end.'.
108+
"\n\n".
109+
'This usually means you have edited a file and left whitespace '.
110+
'characters before the opening %s tag, or after a closing %s tag. '.
111+
'Remove any leading whitespace, and prefer to omit closing tags.',
112+
phutil_tag('tt', array(), '<?php'),
113+
phutil_tag('tt', array(), '?>'));
114+
} else {
115+
$short = id(new PhutilUTF8StringTruncator())
116+
->setMaximumGlyphs(1024)
117+
->truncateString($body);
118+
119+
$message = pht(
120+
'Phabricator sent itself a test request with the '.
121+
'"X-Phabricator-SelfCheck" header and expected to get a valid JSON '.
122+
'response back. Instead, the response begins:'.
123+
"\n\n".
124+
'%s'.
125+
"\n\n".
126+
'Something is misconfigured or otherwise mangling responses.',
127+
phutil_tag('pre', array(), $short));
128+
}
129+
130+
$this->newIssue('webserver.mangle')
131+
->setName(pht('Mangled Webserver Response'))
132+
->setSummary(pht('Your webserver produced an unexpected response.'))
133+
->setMessage($message);
134+
135+
// We can't run the other checks if we could not decode the response.
136+
return;
137+
}
138+
139+
$actual_user = idx($structure, 'user');
140+
$actual_pass = idx($structure, 'pass');
141+
if (($expect_user != $actual_user) || ($actual_pass != $expect_pass)) {
142+
$message = pht(
143+
'Phabricator sent itself a test request with an "Authorization" HTTP '.
144+
'header, and expected those credentials to be transmitted. However, '.
145+
'they were absent or incorrect when received. Phabricator sent '.
146+
'username "%s" with password "%s"; received username "%s" and '.
147+
'password "%s".'.
148+
"\n\n".
149+
'Your webserver may not be configured to forward HTTP basic '.
150+
'authentication. If you plan to use basic authentication (for '.
151+
'example, to access repositories) you should reconfigure it.',
152+
$expect_user,
153+
$expect_pass,
154+
$actual_user,
155+
$actual_pass);
156+
157+
$this->newIssue('webserver.basic-auth')
158+
->setName(pht('HTTP Basic Auth Not Configured'))
159+
->setSummary(pht('Your webserver is not forwarding credentials.'))
160+
->setMessage($message);
161+
}
162+
163+
$actual_path = idx($structure, 'path');
164+
if ($expect_path != $actual_path) {
165+
$message = pht(
166+
'Phabricator sent itself a test request with an unusual path, to '.
167+
'test if your webserver is rewriting paths correctly. The path was '.
168+
'not transmitted correctly.'.
169+
"\n\n".
170+
'Phabricator sent a request to path "%s", and expected the webserver '.
171+
'to decode and rewrite that path so that it received a request for '.
172+
'"%s". However, it received a request for "%s" instead.'.
173+
"\n\n".
174+
'Verify that your rewrite rules are configured correctly, following '.
175+
'the instructions in the documentation. If path encoding is not '.
176+
'working properly you will be unable to access files with unusual '.
177+
'names in repositories, among other issues.'.
178+
"\n\n".
179+
'(This problem can be caused by a missing "B" in your RewriteRule.)',
180+
$send_path,
181+
$expect_path,
182+
$actual_path);
183+
184+
$this->newIssue('webserver.rewrites')
185+
->setName(pht('HTTP Path Rewriting Incorrect'))
186+
->setSummary(pht('Your webserver is rewriting paths improperly.'))
187+
->setMessage($message);
188+
}
189+
190+
$actual_key = pht('<none>');
191+
$actual_value = pht('<none>');
192+
foreach (idx($structure, 'params', array()) as $pair) {
193+
if (idx($pair, 'name') == $expect_key) {
194+
$actual_key = idx($pair, 'name');
195+
$actual_value = idx($pair, 'value');
196+
break;
197+
}
198+
}
199+
200+
if (($expect_key !== $actual_key) || ($expect_value !== $actual_value)) {
201+
$message = pht(
202+
'Phabricator sent itself a test request with an HTTP GET parameter, '.
203+
'but the parameter was not transmitted. Sent "%s" with value "%s", '.
204+
'got "%s" with value "%s".'.
205+
"\n\n".
206+
'Your webserver is configured incorrectly and large parts of '.
207+
'Phabricator will not work until this issue is corrected.'.
208+
"\n\n".
209+
'(This problem can be caused by a missing "QSA" in your RewriteRule.)',
210+
$expect_key,
211+
$expect_value,
212+
$actual_key,
213+
$actual_value);
214+
215+
$this->newIssue('webserver.parameters')
216+
->setName(pht('HTTP Parameters Not Transmitting'))
217+
->setSummary(
218+
pht('Your webserver is not handling GET parameters properly.'))
219+
->setMessage($message);
220+
}
221+
222+
}
223+
224+
}

‎support/PhabricatorStartup.php

+5
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,11 @@ private static function setupPHP() {
395395
if (function_exists('libxml_disable_entity_loader')) {
396396
libxml_disable_entity_loader(true);
397397
}
398+
399+
// Enable automatic compression here. Webservers sometimes do this for
400+
// us, but we now detect the absence of compression and warn users about
401+
// it so try to cover our bases more thoroughly.
402+
ini_set('zlib.output_compression', 1);
398403
}
399404

400405

0 commit comments

Comments
 (0)
Failed to load comments.