From da1d57b60a5f41a3217355c8c44fd9fa6ac2d7c3 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 14 Feb 2012 17:00:20 -0800
Subject: [PATCH] When viewing raw file content in Differential, cache it into
 the File tool before displaying it

Summary:
@alok reported a vulnerability where Flash will run carefully-crafted plain text
files.

When the user requests a raw file, cache it into Files if it isn't already
there. Then redirect them to Files. This solves the problem by executing the
SWF/TXT with CDN-domain permissions, not content-domain permissions, provided
the install is correctly configured. (Followup diff coming to make this more
universally true.)

NOTE: We'll still show raw data in Diffusion. The barrier to XSS here is much
higher (you need commit access) but I'll do something similar there. We aren't
vulnerable in Paste, since we already use Files.

Test Plan: Clicked "View Old File", "View New File" in an alt-domain
configuration, got redirected to a cookie-free domain before being delivered the
response.

Reviewers: btrahan, alok

Reviewed By: btrahan

CC: aran, epriestley

Differential Revision: https://secure.phabricator.com/D1607
---
 .../DifferentialChangesetViewController.php   | 55 +++++++++++++++++--
 .../controller/changesetview/__init__.php     |  3 +-
 2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php b/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php
index 5bdfcdf8ee..d761ffe025 100644
--- a/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php
+++ b/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php
@@ -54,9 +54,9 @@ class DifferentialChangesetViewController extends DifferentialController {
       }
       switch ($view) {
         case 'new':
-          return $this->buildRawFileResponse($changeset->makeNewFile());
+          return $this->buildRawFileResponse($changeset, $is_new = true);
         case 'old':
-          return $this->buildRawFileResponse($changeset->makeOldFile());
+          return $this->buildRawFileResponse($changeset, $is_new = false);
         default:
           return new Aphront400Response();
       }
@@ -256,10 +256,53 @@ class DifferentialChangesetViewController extends DifferentialController {
       $author_phid);
   }
 
-  private function buildRawFileResponse($text) {
-    return id(new AphrontFileResponse())
-      ->setMimeType('text/plain')
-      ->setContent($text);
+  private function buildRawFileResponse(
+    DifferentialChangeset $changeset,
+    $is_new) {
+
+    if ($is_new) {
+      $key = 'raw:new:phid';
+    } else {
+      $key = 'raw:old:phid';
+    }
+
+    $metadata = $changeset->getMetadata();
+
+    $file = null;
+    $phid = idx($metadata, $key);
+    if ($phid) {
+      $file = id(new PhabricatorFile())->loadOneWhere(
+        'phid = %s',
+        $phid);
+    }
+
+    if (!$file) {
+      // This is just building a cache of the changeset content in the file
+      // tool, and is safe to run on a read pathway.
+      $unguard = AphrontWriteGuard::beginScopedUnguardedWrites();
+
+      if ($is_new) {
+        $data = $changeset->makeNewFile();
+      } else {
+        $data = $changeset->makeOldFile();
+      }
+
+      $file = PhabricatorFile::newFromFileData(
+        $data,
+        array(
+          'name'      => $changeset->getFilename(),
+          'mime-type' => 'text/plain',
+        ));
+
+      $metadata[$key] = $file->getPHID();
+      $changeset->setMetadata($metadata);
+      $changeset->save();
+
+      unset($unguard);
+    }
+
+    return id(new AphrontRedirectResponse())
+      ->setURI($file->getBestURI());
   }
 
   private function buildLintInlineComments($changeset) {
diff --git a/src/applications/differential/controller/changesetview/__init__.php b/src/applications/differential/controller/changesetview/__init__.php
index c1564fe51c..ab6be1cc2a 100644
--- a/src/applications/differential/controller/changesetview/__init__.php
+++ b/src/applications/differential/controller/changesetview/__init__.php
@@ -11,8 +11,8 @@ phutil_require_module('arcanist', 'unit/result');
 phutil_require_module('phabricator', 'aphront/response/400');
 phutil_require_module('phabricator', 'aphront/response/404');
 phutil_require_module('phabricator', 'aphront/response/ajax');
-phutil_require_module('phabricator', 'aphront/response/file');
 phutil_require_module('phabricator', 'aphront/response/redirect');
+phutil_require_module('phabricator', 'aphront/writeguard');
 phutil_require_module('phabricator', 'applications/differential/controller/base');
 phutil_require_module('phabricator', 'applications/differential/parser/changeset');
 phutil_require_module('phabricator', 'applications/differential/storage/changeset');
@@ -20,6 +20,7 @@ phutil_require_module('phabricator', 'applications/differential/storage/diffprop
 phutil_require_module('phabricator', 'applications/differential/storage/inlinecomment');
 phutil_require_module('phabricator', 'applications/differential/view/changesetdetailview');
 phutil_require_module('phabricator', 'applications/differential/view/primarypane');
+phutil_require_module('phabricator', 'applications/files/storage/file');
 phutil_require_module('phabricator', 'applications/markup/engine');
 phutil_require_module('phabricator', 'applications/phid/handle/data');
 phutil_require_module('phabricator', 'infrastructure/diff/engine');