diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -15,26 +15,79 @@ $parser = new PhutilQueryStringParser(); $data = array(); - // If the request has "multipart/form-data" content, we can't use - // PhutilQueryStringParser to parse it, and the raw data supposedly is not - // available anyway (according to the PHP documentation, "php://input" is - // not available for "multipart/form-data" requests). However, it is - // available at least some of the time (see T3673), so double check that - // we aren't trying to parse data we won't be able to parse correctly by - // examining the Content-Type header. - $content_type = idx($_SERVER, 'CONTENT_TYPE'); - $is_form_data = preg_match('@^multipart/form-data@i', $content_type); - $request_method = idx($_SERVER, 'REQUEST_METHOD'); if ($request_method === 'PUT') { // For PUT requests, do nothing: in particular, do NOT read input. This // allows us to stream input later and process very large PUT requests, // like those coming from Git LFS. } else { + // For POST requests, we're going to read the raw input ourselves here + // if we can. Among other things, this corrects variable names with + // the "." character in them, which PHP normally converts into "_". + + // There are two major considerations here: whether the + // `enable_post_data_reading` option is set, and whether the content + // type is "multipart/form-data" or not. + + // If `enable_post_data_reading` is off, we're free to read the entire + // raw request body and parse it -- and we must, because $_POST and + // $_FILES are not built for us. If `enable_post_data_reading` is on, + // which is the default, we may not be able to read the body (the + // documentation says we can't, but empirically we can at least some + // of the time). + + // If the content type is "multipart/form-data", we need to build both + // $_POST and $_FILES, which is involved. The body itself is also more + // difficult to parse than other requests. $raw_input = PhabricatorStartup::getRawInput(); - if (strlen($raw_input) && !$is_form_data) { - $data += $parser->parseQueryString($raw_input); + if (strlen($raw_input)) { + $content_type = idx($_SERVER, 'CONTENT_TYPE'); + $is_multipart = preg_match('@^multipart/form-data@i', $content_type); + if ($is_multipart && !ini_get('enable_post_data_reading')) { + $multipart_parser = id(new AphrontMultipartParser()) + ->setContentType($content_type); + + $multipart_parser->beginParse(); + $multipart_parser->continueParse($raw_input); + $parts = $multipart_parser->endParse(); + + $query_string = array(); + foreach ($parts as $part) { + if (!$part->isVariable()) { + continue; + } + + $name = $part->getName(); + $value = $part->getVariableValue(); + + $query_string[] = urlencode($name).'='.urlencode($value); + } + $query_string = implode('&', $query_string); + $post = $parser->parseQueryString($query_string); + + $files = array(); + foreach ($parts as $part) { + if ($part->isVariable()) { + continue; + } + + $files[$part->getName()] = $part->getPHPFileDictionary(); + } + $_FILES = $files; + } else { + $post = $parser->parseQueryString($raw_input); + } + + $_POST = $post; + PhabricatorStartup::rebuildRequest(); + + $data += $post; } else if ($_POST) { + $post = filter_input_array(INPUT_POST, FILTER_UNSAFE_RAW); + if (is_array($post)) { + $_POST = $post; + PhabricatorStartup::rebuildRequest(); + } $data += $_POST; } } diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -178,9 +178,17 @@ } $tmp_name = idx($spec, 'tmp_name'); - $is_valid = @is_uploaded_file($tmp_name); - if (!$is_valid) { - throw new Exception(pht('File is not an uploaded file.')); + + // NOTE: If we parsed the request body ourselves, the files we wrote will + // not be registered in the `is_uploaded_file()` list. It's fine to skip + // this check: it just protects against sloppy code from the long ago era + // of "register_globals". + + if (ini_get('enable_post_data_reading')) { + $is_valid = @is_uploaded_file($tmp_name); + if (!$is_valid) { + throw new Exception(pht('File is not an uploaded file.')); + } } $file_data = Filesystem::readFile($tmp_name); diff --git a/support/PhabricatorStartup.php b/support/PhabricatorStartup.php --- a/support/PhabricatorStartup.php +++ b/support/PhabricatorStartup.php @@ -416,9 +416,12 @@ // NOTE: We don't filter INPUT_SERVER because we don't want to overwrite // changes made in "preamble.php". + + // NOTE: WE don't filter INPUT_POST because we may be constructing it + // lazily if "enable_post_data_reading" is disabled. + $filter = array( INPUT_GET, - INPUT_POST, INPUT_ENV, INPUT_COOKIE, ); @@ -434,9 +437,6 @@ case INPUT_COOKIE: $_COOKIE = array_merge($_COOKIE, $filtered); break; - case INPUT_POST: - $_POST = array_merge($_POST, $filtered); - break; case INPUT_ENV; $env = array_merge($_ENV, $filtered); $_ENV = self::filterEnvSuperglobal($env); @@ -444,18 +444,28 @@ } } - // rebuild $_REQUEST, respecting order declared in ini files + self::rebuildRequest(); + } + + /** + * @task validation + */ + public static function rebuildRequest() { + // Rebuild $_REQUEST, respecting order declared in ".ini" files. $order = ini_get('request_order'); + if (!$order) { $order = ini_get('variables_order'); } + if (!$order) { - // $_REQUEST will be empty, leave it alone + // $_REQUEST will be empty, so leave it alone. return; } + $_REQUEST = array(); - for ($i = 0; $i < strlen($order); $i++) { - switch ($order[$i]) { + for ($ii = 0; $ii < strlen($order); $ii++) { + switch ($order[$ii]) { case 'G': $_REQUEST = array_merge($_REQUEST, $_GET); break; @@ -466,7 +476,7 @@ $_REQUEST = array_merge($_REQUEST, $_COOKIE); break; default: - // $_ENV and $_SERVER never go into $_REQUEST + // $_ENV and $_SERVER never go into $_REQUEST. break; } } @@ -593,6 +603,12 @@ return; } + // If "enable_post_data_reading" is off, we won't have $_POST and this + // condition is effectively impossible. + if (!ini_get('enable_post_data_reading')) { + return; + } + // If there's POST data, clearly we're in good shape. if ($_POST) { return;