diff --git a/conf/default.conf.php b/conf/default.conf.php index 3261b22c6a..5c9ff4ba91 100644 --- a/conf/default.conf.php +++ b/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', + ), ); diff --git a/src/aphront/response/file/AphrontFileResponse.php b/src/aphront/response/file/AphrontFileResponse.php index 0e60435951..4bd2831afd 100644 --- a/src/aphront/response/file/AphrontFileResponse.php +++ b/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, ); } diff --git a/src/applications/files/controller/list/PhabricatorFileListController.php b/src/applications/files/controller/list/PhabricatorFileListController.php index ac2edab897..8e4435baf4 100644 --- a/src/applications/files/controller/list/PhabricatorFileListController.php +++ b/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( - 'a', - array( - 'class' => 'small button grey', - 'href' => '/file/view/'.$file->getPHID().'/', - ), - 'View'), + $view_button, phutil_render_tag( 'a', array( diff --git a/src/applications/files/controller/view/PhabricatorFileViewController.php b/src/applications/files/controller/view/PhabricatorFileViewController.php index 38e9b92654..3dd93248c1 100644 --- a/src/applications/files/controller/view/PhabricatorFileViewController.php +++ b/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()); diff --git a/src/applications/files/storage/file/PhabricatorFile.php b/src/applications/files/storage/file/PhabricatorFile.php index d2ab45826e..e365706304 100644 --- a/src/applications/files/storage/file/PhabricatorFile.php +++ b/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); + } }