From db1cd6586609b53029c45d1aecd7598e942266f1 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 14 May 2015 10:39:28 -0700
Subject: [PATCH] Allow setup checks to perform writes

Summary:
Fixes T8198. Currently, if the `policy.locked` configuration setting includes a value which is a user PHID, we may perform a cache fill during setup as a side effect of validating it.

Right now, there is no WriteGuard active during setup, because we don't have a Request object yet so we can't actually perform CSRF validation.

Two possible approaches are:

  # Prevent the write from occuring.
  # Change the code to allow the write.

In the past, I think we've hit similar cases and done (1). However, IIRC those writes were sketchier, more isolated, and easy to remove (I think there was one with PKCS8 keys). This one is pretty legit and not very easy to remove without making a bit of a mess.

There's no techncial reason we can't do (2), we just have to create a no-op WriteGuard for the setup phase.

Test Plan:
  - To reproduce this issue: set some value in `policy.locked` to a user PHID, then wipe out profile caches in the database, then restart the webserver.
  - Reproduced the issue.
  - Added the new dummy write guard, fixed a minor issue with disposal semantics (see D12841).
  - Verified this fixed the issue.
  - Added a `throw` to the real CSRF validator and performed a real write. Verified I got CSRF-blocked.
  - Removed a CSRF token from a form and double-checked that CSRF protection still works.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8198

Differential Revision: https://secure.phabricator.com/D12842
---
 .../AphrontApplicationConfiguration.php              | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php
index 3e7abe6a9d..d828b9a8ce 100644
--- a/src/aphront/configuration/AphrontApplicationConfiguration.php
+++ b/src/aphront/configuration/AphrontApplicationConfiguration.php
@@ -62,6 +62,11 @@ abstract class AphrontApplicationConfiguration {
     $multimeter->setEventContext('<http-init>');
     $multimeter->setEventViewer('<none>');
 
+    // Build a no-op write guard for the setup phase. We'll replace this with a
+    // real write guard later on, but we need to survive setup and build a
+    // request object first.
+    $write_guard = new AphrontWriteGuard('id');
+
     PhabricatorEnv::initializeWebEnvironment();
 
     $multimeter->setSampleRate(
@@ -108,6 +113,11 @@ abstract class AphrontApplicationConfiguration {
     $application->willBuildRequest();
     $request = $application->buildRequest();
 
+    // Now that we have a request, convert the write guard into one which
+    // actually checks CSRF tokens.
+    $write_guard->dispose();
+    $write_guard = new AphrontWriteGuard(array($request, 'validateCSRF'));
+
     // Build the server URI implied by the request headers. If an administrator
     // has not configured "phabricator.base-uri" yet, we'll use this to generate
     // links.
@@ -121,8 +131,6 @@ abstract class AphrontApplicationConfiguration {
         'U' => (string)$request->getRequestURI()->getPath(),
       ));
 
-    $write_guard = new AphrontWriteGuard(array($request, 'validateCSRF'));
-
     $processing_exception = null;
     try {
       $response = $application->processRequest(