Allow Phabricator to run with "enable_post_data_reading" disabled

Summary:
Ref T13008. Depends on D18701. The overall goal here is to make turning `enable_post_data_reading` off not break things, so we can run rate limiting checks before we read file uploads.

The biggest blocker for this is that turning it off stops `$_FILES` from coming into existence.

This //appears// to mostly work. Specifically:

  - Skip the `max_post_size` check when POST is off, since it's meaningless.
  - Don't read or scrub $_POST at startup when POST is off.
  - When we rebuild REQUEST and POST before processing requests, do multipart parsing if we need to and rebuild FILES.
  - Skip the `is_uploaded_file()` check if we built FILES ourselves.

This probably breaks a couple of small things, like maybe `__profile__` and other DarkConsole triggers over POST, and probably some other weird stuff. The parsers may also need more work than they've received so far.

I also need to verify that this actually works (i.e., lets us run code without reading the request body) but I'll include that in the change where I update the actual rate limiting.

Test Plan:
  - Disabled `enable_post_data_reading`.
  - Uploaded a file with a vanilla upload form (project profile image).
  - Uploaded a file with drag and drop.
  - Used DarkConsole.
  - Submitted comments.
  - Created a task.
  - Browsed around.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13008

Differential Revision: https://secure.phabricator.com/D18702

src/aphront/configuration/AphrontDefaultApplicationConfiguration.php

@@ -15,26 +15,79 @@ class AphrontDefaultApplicationConfiguration
     $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;
       }
     }

src/applications/files/storage/PhabricatorFile.php

@@ -178,10 +178,18 @@ final class PhabricatorFile extends PhabricatorFileDAO
     }
 
     $tmp_name = idx($spec, 'tmp_name');
+
+    // 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);
     $file_size = idx($spec, 'size');

support/PhabricatorStartup.php

@@ -416,9 +416,12 @@ final class PhabricatorStartup {
 
     // 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 @@ final class PhabricatorStartup {
         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 @@ final class PhabricatorStartup {
       }
     }
 
-    // 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 @@ final class PhabricatorStartup {
           $_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 @@ final class PhabricatorStartup {
       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;