revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 14:00:56 +01:00
Code Issues Releases Wiki Activity

Refine the "Mangled Webserver Response" setup check

Browse source 
Summary:
Ref T13259. In some configurations, making a request to ourselves may return a VPN/Auth response from some LB/appliance layer.

If this response begins or ends with whitespace, we currently detect it as "extra whitespace" instead of "bad response".

Instead, require that the response be nearly correct (valid JSON with some extra whitespace, instead of literally anything with some extra whitespace) to hit this specialized check. If we don't hit the specialized case, use the generic "mangled" response error, which prints the actual body so you can figure out that it's just your LB/auth thing doing what it's supposed to do.

Test Plan:
  - Rigged responses to add extra whitespace, got "Extra Whitespace" (same as before).
  - Rigged responses to add extra non-whitespace, got "Mangled Junk" (same as before).
  - Rigged responses to add extra whitespace and extra non-whitespace, got "Mangled Junk" with a sample of the document body instead of "Extra Whitespace" (improvement).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13259

Differential Revision: https://secure.phabricator.com/D20235
This commit is contained in:
epriestley 2019-02-28 19:41:56 -08:00
parent c116deef63
commit ea6c0c9bde
2 changed files with 20 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/aphront/configuration/AphrontApplicationConfiguration.php
View file

@ -776,7 +776,6 @@ final class AphrontApplicationConfiguration
'filler' => str_repeat('Q', 1024 * 16),
);
return id(new AphrontJSONResponse())
->setAddJSONShield(false)
->setContent($result);

41
src/applications/config/check/PhabricatorWebServerSetupCheck.php
View file

@ -129,30 +129,16 @@ final class PhabricatorWebServerSetupCheck extends PhabricatorSetupCheck {
}
$structure = null;
$caught = null;
$extra_whitespace = ($body !== trim($body));
if (!$extra_whitespace) {
try {
$structure = phutil_json_decode($body);
} catch (Exception $ex) {
$caught = $ex;
}
try {
$structure = phutil_json_decode(trim($body));
} catch (Exception $ex) {
// Ignore the exception, we only care if the decode worked or not.
}
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(), '<?php'),
phutil_tag('tt', array(), '?>'));
} else {
if (!$structure || $extra_whitespace) {
if (!$structure) {
$short = id(new PhutilUTF8StringTruncator())
->setMaximumGlyphs(1024)
->truncateString($body);
@ -166,6 +152,17 @@ final class PhabricatorWebServerSetupCheck extends PhabricatorSetupCheck {
"\n\n".
'Something is misconfigured or otherwise mangling responses.',
phutil_tag('pre', array(), $short));
} else {
$message = pht(
'Phabricator sent itself a test request and expected to get a bare '.
'JSON response back. It received a JSON response, 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(), '<?php'),
phutil_tag('tt', array(), '?>'));
}
$this->newIssue('webserver.mangle')
@ -174,7 +171,9 @@ final class PhabricatorWebServerSetupCheck extends PhabricatorSetupCheck {
->setMessage($message);
// We can't run the other checks if we could not decode the response.
return;
if (!$structure) {
return;
}
}
$actual_user = idx($structure, 'user');