From 0b9c54a6bb780425a49af9a91bffe695be0a8908 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Dec 2012 14:01:18 -0800
Subject: [PATCH] Detect missing 'params' in Conduit calls

Summary:
Suhosin has about 50 options for filtering input variables, doucmented here:

http://www.hardened-php.net/suhosin/configuration.html

The default behavior of Suhosin is to drop the variable entirely if it violates any of the rules, then continue with the request. It doesn't affect 'php://input' and doesn't drop other variables, so it evades existing detection, and we can't figure out that it's happened at runtime. We could add blanket checks (Suhosin enabled + suhosin.filter.action set to nothing means this may happen, and will be undetectable if it does happen) but can't tailor a check or recovery to this specific problem.

Instead, raise a better error in the specific case where we encounter this, which is Conduit calls of "arc diff" of files over 1MB (the default POST limit). In these cases, Suhosin drops the variable entirely. If there is no 'params', scream. We never encounter this case normall (`arc`, including `arc call-conduit`, always sends this parameter) although other clients might omit it. The only exception is the web console with `conduit.ping`, which submits nothing; make it submit something so it keeps working.

See also https://github.com/facebook/phabricator/issues/233#issuecomment-11186074

Test Plan: Brought up a Debian + Suhosin box, verified the behavior of Suhosin, made requests with and without 'params'.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D4144
---
 .../controller/PhabricatorConduitAPIController.php  | 13 ++++++++++++-
 .../PhabricatorConduitConsoleController.php         |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
index 21cca29a0b..7b9ea60e56 100644
--- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php
+++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
@@ -451,7 +451,18 @@ final class PhabricatorConduitAPIController
 
     $params_json = $request->getStr('params');
     if (!strlen($params_json)) {
-      $params = array();
+      if ($request->getBool('allowEmptyParams')) {
+        // TODO: This is a bit messy, but otherwise you can't call
+        // "conduit.ping" from the web console.
+        $params = array();
+      } else {
+        throw new Exception(
+          "Request has no 'params' key. This may mean that an extension like ".
+          "Suhosin has dropped data from the request. Check the PHP ".
+          "configuration on your server. If you are developing a Conduit ".
+          "client, you MUST provide a 'params' parameter when making a ".
+          "Conduit request, even if the value is empty (e.g., provide '{}').");
+      }
     } else {
       $params = json_decode($params_json, true);
       if (!is_array($params)) {
diff --git a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php
index e8bcc28592..3d0f2c41a0 100644
--- a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php
+++ b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php
@@ -74,6 +74,7 @@ final class PhabricatorConduitConsoleController
     $form
       ->setUser($request->getUser())
       ->setAction('/api/'.$this->method)
+      ->addHiddenInput('allowEmptyParams', 1)
       ->appendChild(
         id(new AphrontFormStaticControl())
           ->setLabel('Description')