From 772a94236644e290c1c9b34eabe5a9b9fd0c2ddc Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 17 Aug 2012 13:41:57 -0700 Subject: [PATCH] Detect 'post_max_size' more robustly Summary: Currently, when a user runs "arc diff" and the diff exceeds PHP's 'post_max_size', they get a very confusing and irrelevant error about a missing Conduit session token. The reason for this is that 'post_max_size' doesn't build $_POST, so //all// the data is missing. We try to detect this, but currently only do so effectively for specific file upload forms. Broaden the detection to cover all cases. Previously, we ran into an issue where Firefox + HTML5 drag-and-drop uploads would get a false positive on this detection. I dug into this and added the Content-Type checks, which correctly handle that case. Test Plan: With small and large 'post_max_size', ran small and large normal, HTML5 and multipart/form-data POST requests against Phabricator in Safari and Firefox. Got desired beahviors. Reviewers: vrana, btrahan Reviewed By: btrahan CC: tido, aran Differential Revision: https://secure.phabricator.com/D3320 --- src/__phutil_library_map__.php | 1 + .../PhabricatorRequestOverseer.php | 110 ++++++++++++++++++ webroot/index.php | 10 +- 3 files changed, 114 insertions(+), 7 deletions(-) create mode 100644 src/infrastructure/PhabricatorRequestOverseer.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1e3f291b77..f93ada52f9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -985,6 +985,7 @@ phutil_register_library_map(array( 'PhabricatorRepositorySymbol' => 'applications/repository/storage/PhabricatorRepositorySymbol.php', 'PhabricatorRepositoryTestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php', 'PhabricatorRepositoryType' => 'applications/repository/constants/PhabricatorRepositoryType.php', + 'PhabricatorRequestOverseer' => 'infrastructure/PhabricatorRequestOverseer.php', 'PhabricatorS3FileStorageEngine' => 'applications/files/engine/PhabricatorS3FileStorageEngine.php', 'PhabricatorSQLPatchList' => 'infrastructure/storage/patch/PhabricatorSQLPatchList.php', 'PhabricatorScopedEnv' => 'infrastructure/PhabricatorScopedEnv.php', diff --git a/src/infrastructure/PhabricatorRequestOverseer.php b/src/infrastructure/PhabricatorRequestOverseer.php new file mode 100644 index 0000000000..0c2b28d274 --- /dev/null +++ b/src/infrastructure/PhabricatorRequestOverseer.php @@ -0,0 +1,110 @@ +detectPostMaxSizeTriggered(); + } + + /** + * Detect if this request has had its POST data stripped by exceeding the + * 'post_max_size' PHP configuration limit. + * + * PHP has a setting called 'post_max_size'. If a POST request arrives with + * a body larger than the limit, PHP doesn't generate $_POST but processes + * the request anyway, and provides no formal way to detect that this + * happened. + * + * We can still read the entire body out of `php://input`. However, this + * stream can't be rewound, and according to the documentation isn't available + * for "multipart/form-data" (on nginx + php-fpm it appears that it is + * available, though, at least) so any attempt to generate $_POST would create + * side effects and be fragile. + */ + private function detectPostMaxSizeTriggered() { + // If this wasn't a POST, we're fine. + if ($_SERVER['REQUEST_METHOD'] != 'POST') { + return; + } + + // If there's POST data, clearly we're in good shape. + if ($_POST) { + return; + } + + // PHP generates $_POST only for two content types. This routing happens + // in `main/php_content_types.c` in PHP. Normally, all forms use one of + // these content types, but some requests may not -- for example, Firefox + // submits files sent over HTML5 XMLHTTPRequest APIs with the Content-Type + // of the file itself. If we don't have a recognized content type, we + // don't need $_POST. + // + // NOTE: We use strncmp() because the actual content type may be something + // like "multipart/form-data; boundary=...". + $content_type = $_SERVER['CONTENT_TYPE']; + + $parsed_types = array( + 'application/x-www-form-urlencoded', + 'multipart/form-data', + ); + + $is_parsed_type = false; + foreach ($parsed_types as $parsed_type) { + if (strncmp($content_type, $parsed_type, strlen($parsed_type)) === 0) { + $is_parsed_type = true; + break; + } + } + + if (!$is_parsed_type) { + return; + } + + // Check for 'Content-Length'. If there's no data, we don't expect $_POST + // to exist. + $length = (int)$_SERVER['CONTENT_LENGTH']; + if (!$length) { + return; + } + + // Time to fatal: we know this was a POST with data that should have been + // populated into $_POST, but it wasn't. + + $config = ini_get('post_max_size'); + $this->fatal( + "As received by the server, this request had a nonzero content length ". + "but no POST data.\n\n". + "Normally, this indicates that it exceeds the 'post_max_size' setting ". + "in the PHP configuration on the server. Increase the 'post_max_size' ". + "setting or reduce the size of the request.\n\n". + "Request size according to 'Content-Length' was '{$length}', ". + "'post_max_size' is set to '{$config}'."); + } + + /** + * Defined in webroot/index.php. + * TODO: Move here. + * + * @phutil-external-symbol function phabricator_fatal + */ + public function fatal($message) { + phabricator_fatal('FATAL ERROR: '.$message); + } + +} diff --git a/webroot/index.php b/webroot/index.php index 484652b3f6..fbabeb7e7c 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -21,13 +21,6 @@ $access_log = null; error_reporting(E_ALL | E_STRICT); -if ($_SERVER['REQUEST_METHOD'] == 'POST' && !$_POST && - isset($_REQUEST['__file__'])) { - $size = ini_get('post_max_size'); - phabricator_fatal( - "Request size exceeds PHP 'post_max_size' ('{$size}')."); -} - $required_version = '5.2.3'; if (version_compare(PHP_VERSION, $required_version) < 0) { phabricator_fatal_config_error( @@ -77,6 +70,9 @@ require_once dirname(dirname(__FILE__)).'/conf/__init_conf__.php'; try { setup_aphront_basics(); + $overseer = new PhabricatorRequestOverseer(); + $overseer->didStartup(); + $conf = phabricator_read_config_file($env); $conf['phabricator.env'] = $env;