From a88dc2afc2a9854dab25df28351109f5aec6607d Mon Sep 17 00:00:00 2001
From: Josh Cox <joshcox@uberatc.com>
Date: Thu, 25 Aug 2016 12:54:10 -0400
Subject: [PATCH] Added a setup check for empty REMOTE_ADDR

Summary: Fixes T8850. Previously, if a user's preamble script mangled `$_SERVER['REMOTE_ADDR']` or somehow set it to `null`, the user would get errors when performing certain actions. Now those errors shouldn't occur, and instead the user will be warned that there is a setup issue related to their preamble script.

Test Plan: Create a preamble script that contains `$_SERVER['REMOTE_ADDR'] = null;` then navigate to /config/issue/. There should be a warning there about `REMOTE_ADDR` not being available.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, yelirekim, epriestley

Maniphest Tasks: T8850

Differential Revision: https://secure.phabricator.com/D16450
---
 .../check/PhabricatorPHPConfigSetupCheck.php  | 27 +++++++++++++++++++
 .../people/storage/PhabricatorUserLog.php     |  2 +-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/src/applications/config/check/PhabricatorPHPConfigSetupCheck.php b/src/applications/config/check/PhabricatorPHPConfigSetupCheck.php
index 6a08ec68ae..b3fd03fd19 100644
--- a/src/applications/config/check/PhabricatorPHPConfigSetupCheck.php
+++ b/src/applications/config/check/PhabricatorPHPConfigSetupCheck.php
@@ -197,5 +197,32 @@ final class PhabricatorPHPConfigSetupCheck extends PhabricatorSetupCheck {
           ->setMessage($message);
       }
     }
+
+    if (empty($_SERVER['REMOTE_ADDR'])) {
+      $doc_href = PhabricatorEnv::getDocLink('Configuring a Preamble Script');
+
+      $summary = pht(
+        'You likely need to fix your preamble script so '.
+        'REMOTE_ADDR is no longer empty.');
+
+      $message = pht(
+        'No REMOTE_ADDR is available, so Phabricator cannot determine the '.
+        'origin address for requests. This will prevent Phabricator from '.
+        'performing important security checks. This most often means you '.
+        'have a mistake in your preamble script. Consult the documentation '.
+        '(%s) and double-check that the script is written correctly.',
+        phutil_tag(
+          'a',
+          array(
+            'href' => $doc_href,
+            'target' => '_blank',
+            ),
+          pht('Configuring a Preamble Script')));
+
+      $this->newIssue('php.remote_addr')
+        ->setName(pht('No REMOTE_ADDR available'))
+        ->setSummary($summary)
+        ->setMessage($message);
+    }
   }
 }
diff --git a/src/applications/people/storage/PhabricatorUserLog.php b/src/applications/people/storage/PhabricatorUserLog.php
index 5939778f25..9727c81c63 100644
--- a/src/applications/people/storage/PhabricatorUserLog.php
+++ b/src/applications/people/storage/PhabricatorUserLog.php
@@ -108,7 +108,7 @@ final class PhabricatorUserLog extends PhabricatorUserDAO
     $log->setUserPHID((string)$object_phid);
     $log->setAction($action);
 
-    $log->remoteAddr = idx($_SERVER, 'REMOTE_ADDR', '');
+    $log->remoteAddr = (string)idx($_SERVER, 'REMOTE_ADDR', '');
 
     return $log;
   }