Security: tighten up the File tool against clowning around.

Summary:

Test Plan:

Reviewers:

CC:

conf/default.conf.php

@@ -233,5 +233,25 @@ return array(
   // Version string displayed in the footer. You probably should leave this
   // alone.
   'phabricator.version'         => 'UNSTABLE',
+  
+
+// -- Files ----------------------------------------------------------------- //
+
+  // Lists which uploaded file types may be viewed in the browser. If a file
+  // has a mime type which does not appear in this list, it will always be
+  // downloaded instead of displayed. This is a security consideration: if a
+  // user uploads a file of type "text/html" and it is displayed as
+  // "text/html", they can eaily execute XSS attacks. This is also a usability
+  // consideration, since browsers tend to freak out when viewing enormous
+  // binary files.
+  //
+  // The keys in this array are viewable mime types; the values are the mime
+  // types they will be delivered as when they are viewed in the browser.
+  'files.viewable-mime-types'   => array(
+    'image/jpeg'  => 'image/jpeg',
+    'image/jpg'   => 'image/jpg',
+    'image/png'   => 'image/png',
+    'text/plain'  => 'text/plain; charset=utf-8',
+  ),
 
 );

src/aphront/response/file/AphrontFileResponse.php

@@ -26,6 +26,10 @@ class AphrontFileResponse extends AphrontResponse {
   private $download;
 
   public function setDownload($download) {
+    $download = preg_replace('/[^A-Za-z0-9_.-]/', '_', $download);
+    if (!strlen($download)) {
+      $download = 'untitled_document.txt';
+    }
     $this->download = $download;
     return $this;
   }
@@ -55,12 +59,20 @@ class AphrontFileResponse extends AphrontResponse {
   public function getHeaders() {
     $headers = array(
       array('Content-Type', $this->getMimeType()),
+      // Without this, IE can decide that we surely meant "text/html" when
+      // delivering another content type since, you know, it looks like it's
+      // probably an HTML document. This closes the security hole that policy
+      // creates.
+      array('X-Content-Type-Options', 'nosniff'),
     );
 
-    if ($this->getDownload()) {
+    if (strlen($this->getDownload())) {
+      $headers[] = array('X-Download-Options', 'noopen');
+      
+      $filename = $this->getDownload();
       $headers[] = array(
         'Content-Disposition',
-        'attachment; filename='.$this->getDownload(),
+        'attachment; filename='.$filename,
       );
     }
 

src/applications/files/controller/list/PhabricatorFileListController.php

@@ -24,6 +24,17 @@ class PhabricatorFileListController extends PhabricatorFileController {
 
     $rows = array();
     foreach ($files as $file) {
+      if ($file->isViewableInBrowser()) {
+        $view_button = phutil_render_tag(
+          'a',
+          array(
+            'class' => 'small button grey',
+            'href'  => '/file/view/'.$file->getPHID().'/',
+          ),
+          'View');
+      } else {
+        $view_button = null;
+      } 
       $rows[] = array(
         phutil_escape_html($file->getPHID()),
         phutil_escape_html($file->getName()),
@@ -35,13 +46,7 @@ class PhabricatorFileListController extends PhabricatorFileController {
             'href'  => '/file/info/'.$file->getPHID().'/',
           ),
           'Info'),
-        phutil_render_tag(
+        $view_button,
-          'a',
-          array(
-            'class' => 'small button grey',
-            'href'  => '/file/view/'.$file->getPHID().'/',
-          ),
-          'View'),
         phutil_render_tag(
           'a',
           array(

src/applications/files/controller/view/PhabricatorFileViewController.php

@@ -34,15 +34,32 @@ class PhabricatorFileViewController extends PhabricatorFileController {
     if (!$file) {
       return new Aphront404Response();
     }
-
+    
     switch ($this->view) {
       case 'download':
       case 'view':
         $data = $file->loadFileData();
         $response = new AphrontFileResponse();
         $response->setContent($data);
-        $response->setMimeType($file->getMimeType());
+        
-        if ($this->view == 'download') {
+        if ($this->view == 'view') {
+          if (!$file->isViewableInBrowser()) {
+            return new Aphront400Response();
+          }
+          $download = false;
+        } else {
+          $download = true;
+        }
+        
+        if ($download) {
+          $mime_type = $file->getMimeType();
+        } else {
+          $mime_type = $file->getViewableMimeType();
+        }
+
+        $response->setMimeType($mime_type);
+        
+        if ($download) {
           $response->setDownload($file->getName());
         }
         return $response;
@@ -51,7 +68,14 @@ class PhabricatorFileViewController extends PhabricatorFileController {
     }
 
     $form = new AphrontFormView();
-    $form->setAction('/file/view/'.$file->getPHID().'/');
+    
+    if ($file->isViewableInBrowser()) {
+      $form->setAction('/file/view/'.$file->getPHID().'/');
+      $button_name = 'View File';
+    } else {
+      $form->setAction('/file/download/'.$file->getPHID().'/');
+      $button_name = 'Download File';
+    }
     $form->setUser($this->getRequest()->getUser());
     $form
       ->appendChild(
@@ -96,7 +120,7 @@ class PhabricatorFileViewController extends PhabricatorFileController {
           ->setValue($file->getStorageHandle()))
       ->appendChild(
         id(new AphrontFormSubmitControl())
-          ->setValue('View File'));
+          ->setValue($button_name));
 
     $panel = new AphrontPanelView();
     $panel->setHeader('File Info - '.$file->getName());

src/applications/files/storage/file/PhabricatorFile.php

@@ -174,5 +174,19 @@ class PhabricatorFile extends PhabricatorFileDAO {
   public function getViewURI() {
     return PhabricatorFileURI::getViewURIForPHID($this->getPHID());
   }
+  
+  public function isViewableInBrowser() {
+    return ($this->getViewableMimeType() !== null);
+  }
+    
+  public function getViewableMimeType() {
+    $mime_map = PhabricatorEnv::getEnvConfig('files.viewable-mime-types');
+
+    $mime_type = $this->getMimeType();
+    $mime_parts = explode(';', $mime_type);
+    $mime_type = reset($mime_parts);
+  
+    return idx($mime_map, $mime_type);
+  }
 
 }