From 0a4cbdff5e597545740ec567ec539e2e3607633e Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 19 Mar 2012 19:52:24 -0700 Subject: [PATCH] Straighten out Diffusion file integration Summary: This is in preparation for getting the "View Options" dropdown working on audits. - Use Files to serve raw data so we get all the security benefits of the alternate file domain. Although the difficulty of exploiting this is high (you need commit access to the repo) there's no reason to leave it dangling. - Add a "contentHash" to Files so we can lookup files by content rather than adding some weird linker table. We can do other things with this later, potentially. - Don't use 'data' URIs since they're crazy and we can just link to the file URI. - When showing a binary file or an image, don't give options like "show highlighted text with blame" or "edit in external editor" since they don't make any sense. - Use the existing infrastructure to figure out if things are images or binaries instead of an ad-hoc thing in this class. Test Plan: Looked at text, image and binary files in Diffusion. Verified we reuse existing files if we've already generated them. Reviewers: btrahan, vrana Reviewed By: btrahan CC: aran, epriestley Maniphest Tasks: T904 Differential Revision: https://secure.phabricator.com/D1899 --- conf/default.conf.php | 13 + resources/sql/patches/119.filehash.sql | 5 + .../file/DiffusionBrowseFileController.php | 283 +++++++++--------- .../diffusion/controller/file/__init__.php | 8 +- .../files/storage/file/PhabricatorFile.php | 12 + .../files/storage/file/__init__.php | 1 + .../mysql/AphrontMySQLDatabaseConnection.php | 2 +- 7 files changed, 180 insertions(+), 144 deletions(-) create mode 100644 resources/sql/patches/119.filehash.sql diff --git a/conf/default.conf.php b/conf/default.conf.php index 75e4bb2d63..abbdb7bf2e 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -539,6 +539,19 @@ return array( 'image/vnd.microsoft.icon' => 'image/x-icon', ), + // List of mime types which can be used as the source for an tag. + // This should be a subset of 'files.viewable-mime-types' and exclude files + // like text. + 'files.image-mime-types' => array( + 'image/jpeg' => true, + 'image/jpg' => true, + 'image/png' => true, + 'image/gif' => true, + 'image/x-ico' => true, + 'image/x-icon' => true, + 'image/vnd.microsoft.icon' => true, + ), + // Phabricator can proxy images from other servers so you can paste the URI // to a funny picture of a cat into the comment box and have it show up as an // image. However, this means the webserver Phabricator is running on will diff --git a/resources/sql/patches/119.filehash.sql b/resources/sql/patches/119.filehash.sql new file mode 100644 index 0000000000..549721165c --- /dev/null +++ b/resources/sql/patches/119.filehash.sql @@ -0,0 +1,5 @@ +ALTER TABLE phabricator_file.file + ADD contentHash varchar(40) COLLATE utf8_bin; + +ALTER TABLE phabricator_file.file + ADD KEY (contentHash); diff --git a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php index bf559965c4..c78c106dcd 100644 --- a/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/file/DiffusionBrowseFileController.php @@ -18,32 +18,10 @@ final class DiffusionBrowseFileController extends DiffusionController { - // Image types we want to display inline using tags - protected $imageTypes = array( - 'png' => 'image/png', - 'gif' => 'image/gif', - 'ico' => 'image/png', - 'jpg' => 'image/jpeg', - 'jpeg'=> 'image/jpeg' - ); - - // Document types that should trigger link to ?view=raw - protected $documentTypes = array( - 'pdf'=> 'application/pdf', - 'ps' => 'application/postscript', - ); + private $corpusType = 'text'; public function processRequest() { - // Build the view selection form. - $select_map = array( - 'highlighted' => 'View as Highlighted Text', - 'blame' => 'View as Highlighted Text with Blame', - 'plain' => 'View as Plain Text', - 'plainblame' => 'View as Plain Text with Blame', - 'raw' => 'View as raw document', - ); - $request = $this->getRequest(); $drequest = $this->getDiffusionRequest(); @@ -55,67 +33,11 @@ final class DiffusionBrowseFileController extends DiffusionController { $file_query->setNeedsBlame($needs_blame); $file_query->loadFileContent(); $data = $file_query->getRawData(); + if ($selected === 'raw') { - $response = new AphrontFileResponse(); - $response->setContent($data); - $mime_type = $this->getDocumentType($path); - if ($mime_type) { - $response->setMimeType($mime_type); - } else { - $as_filename = idx(pathinfo($path), 'basename'); - $response->setDownload($as_filename); - } - return $response; + return $this->buildRawResponse($path, $data); } - $select = ''; - - require_celerity_resource('diffusion-source-css'); - - $edit_button = ''; - $user = $request->getUser(); - if ($user) { - $line = 1; - $repository = $this->getDiffusionRequest()->getRepository(); - $editor_link = $user->loadEditorLink($path, $line, $repository); - if ($editor_link) { - $edit_button = phutil_render_tag( - 'a', - array( - 'href' => $editor_link, - 'class' => 'button', - ), - 'Edit'); - } - } - - $view_select_panel = new AphrontPanelView(); - $view_select_form = phutil_render_tag( - 'form', - array( - 'action' => $request->getRequestURI(), - 'method' => 'get', - 'class' => 'diffusion-browse-type-form', - ), - $select. - ' '. - $edit_button); - $view_select_panel->appendChild($view_select_form); - - $view_select_panel->appendChild('
'); - // Build the content of the file. $corpus = $this->buildCorpus( $selected, @@ -123,8 +45,15 @@ final class DiffusionBrowseFileController extends DiffusionController { $needs_blame, $drequest, $path, - $data - ); + $data); + + require_celerity_resource('diffusion-source-css'); + + if ($this->corpusType == 'text') { + $view_select_panel = $this->renderViewSelectPanel(); + } else { + $view_select_panel = null; + } // Render the page. $content = array(); @@ -141,7 +70,6 @@ final class DiffusionBrowseFileController extends DiffusionController { $nav = $this->buildSideNav('browse', true); $nav->appendChild($content); - $basename = basename($this->getDiffusionRequest()->getPath()); return $this->buildStandardPageResponse( @@ -151,71 +79,24 @@ final class DiffusionBrowseFileController extends DiffusionController { )); } - - /** - * Returns a content-type corrsponding to an image file extension - * - * @param string $path File path - * @return mixed A content-type string or NULL if path doesn't end with a - * recognized image extension - */ - public function getImageType($path) { - $ext = pathinfo($path); - $ext = idx($ext, 'extension'); - return idx($this->imageTypes, $ext); - } - - /** - * Returns a content-type corresponding to an document file extension - * - * @param string $path File path - * @return mixed A content-type string or NULL if path doesn't end with a - * recognized document extension - */ - public function getDocumentType($path) { - $ext = pathinfo($path); - $ext = idx($ext, 'extension'); - return idx($this->documentTypes, $ext); - } - - private function buildCorpus($selected, $file_query, $needs_blame, $drequest, $path, $data) { - $image_type = $this->getImageType($path); - if ($image_type && !$selected) { - $corpus = phutil_render_tag( - 'img', - array( - 'style' => 'padding-bottom: 10px', - 'src' => 'data:'.$image_type.';base64,'.base64_encode($data), - ) - ); - return $corpus; - } - $document_type = $this->getDocumentType($path); - if (($document_type && !$selected) || !phutil_is_utf8($data)) { - $data = $file_query->getRawData(); - $document_type_description = $document_type ? $document_type : 'binary'; - $corpus = phutil_render_tag( - 'p', - array( - 'style' => 'text-align: center;' - ), - phutil_render_tag( - 'a', - array( - 'href' => '?view=raw', - 'class' => 'button' - ), - "View $document_type_description" - ) - ); - return $corpus; + if (ArcanistDiffUtils::isHeuristicBinaryFile($data)) { + $file = $this->loadFileForData($path, $data); + $file_uri = $file->getBestURI(); + + if ($file->isViewableImage()) { + $this->corpusType = 'image'; + return $this->buildImageCorpus($file_uri); + } else { + $this->corpusType = 'binary'; + return $this->buildBinaryCorpus($file_uri, $data); + } } @@ -295,6 +176,64 @@ final class DiffusionBrowseFileController extends DiffusionController { return $corpus; } + private function renderViewSelectPanel() { + + $request = $this->getRequest(); + + $select = AphrontFormSelectControl::renderSelectTag( + $request->getStr('view'), + array( + 'highlighted' => 'View as Highlighted Text', + 'blame' => 'View as Highlighted Text with Blame', + 'plain' => 'View as Plain Text', + 'plainblame' => 'View as Plain Text with Blame', + 'raw' => 'View as raw document', + ), + array( + 'name' => 'view', + )); + + $view_select_panel = new AphrontPanelView(); + $view_select_form = phutil_render_tag( + 'form', + array( + 'action' => $request->getRequestURI(), + 'method' => 'get', + 'class' => 'diffusion-browse-type-form', + ), + $select. + ' '. + $this->renderEditButton()); + + $view_select_panel->appendChild($view_select_form); + $view_select_panel->appendChild('
'); + + return $view_select_panel; + } + + private function renderEditButton() { + $request = $this->getRequest(); + $user = $request->getUser(); + + $drequest = $this->getDiffusionRequest(); + + $repository = $drequest->getRepository(); + $path = $drequest->getPath(); + $line = 1; + + $editor_link = $user->loadEditorLink($path, $line, $repository); + if (!$editor_link) { + return null; + } + + return phutil_render_tag( + 'a', + array( + 'href' => $editor_link, + 'class' => 'button', + ), + 'Edit'); + } private function buildDisplayRows($text_list, $rev_list, $blame_dict, $needs_blame, DiffusionRequest $drequest, $file_query, $selected) { @@ -486,5 +425,65 @@ final class DiffusionBrowseFileController extends DiffusionController { ); } + private function loadFileForData($path, $data) { + $hash = PhabricatorHash::digest($data); + + $file = id(new PhabricatorFile())->loadOneWhere( + 'contentHash = %s LIMIT 1', + $hash); + if (!$file) { + // We're just caching the data; this is always safe. + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + + $file = PhabricatorFile::newFromFileData( + $data, + array( + 'name' => basename($path), + )); + + unset($unguarded); + } + + return $file; + } + + private function buildRawResponse($path, $data) { + $file = $this->loadFileForData($path, $data); + return id(new AphrontRedirectResponse())->setURI($file->getBestURI()); + } + + private function buildImageCorpus($file_uri) { + $panel = new AphrontPanelView(); + $panel->setHeader('Image'); + $panel->addButton($this->renderEditButton()); + $panel->appendChild( + phutil_render_tag( + 'img', + array( + 'src' => $file_uri, + ))); + return $panel; + } + + private function buildBinaryCorpus($file_uri, $data) { + $panel = new AphrontPanelView(); + $panel->setHeader('Binary File'); + $panel->addButton($this->renderEditButton()); + $panel->appendChild( + '

'. + 'This is a binary file. '. + 'It is '.number_format(strlen($data)).' bytes in length.'. + '

'); + $panel->addButton( + phutil_render_tag( + 'a', + array( + 'href' => $file_uri, + 'class' => 'button green', + ), + 'Download Binary File...')); + return $panel; + } + } diff --git a/src/applications/diffusion/controller/file/__init__.php b/src/applications/diffusion/controller/file/__init__.php index bc661ff283..6ed4027961 100644 --- a/src/applications/diffusion/controller/file/__init__.php +++ b/src/applications/diffusion/controller/file/__init__.php @@ -6,12 +6,18 @@ -phutil_require_module('phabricator', 'aphront/response/file'); +phutil_require_module('arcanist', 'difference'); + +phutil_require_module('phabricator', 'aphront/response/redirect'); +phutil_require_module('phabricator', 'aphront/writeguard'); phutil_require_module('phabricator', 'applications/diffusion/controller/base'); phutil_require_module('phabricator', 'applications/diffusion/query/filecontent/base'); +phutil_require_module('phabricator', 'applications/files/storage/file'); phutil_require_module('phabricator', 'applications/markup/syntax'); phutil_require_module('phabricator', 'infrastructure/celerity/api'); phutil_require_module('phabricator', 'infrastructure/javelin/api'); +phutil_require_module('phabricator', 'infrastructure/util/hash'); +phutil_require_module('phabricator', 'view/form/control/select'); phutil_require_module('phabricator', 'view/layout/panel'); phutil_require_module('phabricator', 'view/utils'); diff --git a/src/applications/files/storage/file/PhabricatorFile.php b/src/applications/files/storage/file/PhabricatorFile.php index 20fb09c7e0..adc35c4edd 100644 --- a/src/applications/files/storage/file/PhabricatorFile.php +++ b/src/applications/files/storage/file/PhabricatorFile.php @@ -26,6 +26,7 @@ final class PhabricatorFile extends PhabricatorFileDAO { protected $byteSize; protected $authorPHID; protected $secretKey; + protected $contentHash; protected $storageEngine; protected $storageFormat; @@ -138,6 +139,7 @@ final class PhabricatorFile extends PhabricatorFileDAO { $file->setName($file_name); $file->setByteSize(strlen($data)); $file->setAuthorPHID($authorPHID); + $file->setContentHash(PhabricatorHash::digest($data)); $file->setStorageEngine($engine_identifier); $file->setStorageHandle($data_handle); @@ -254,6 +256,16 @@ final class PhabricatorFile extends PhabricatorFileDAO { return ($this->getViewableMimeType() !== null); } + public function isViewableImage() { + if (!$this->isViewableInBrowser()) { + return false; + } + + $mime_map = PhabricatorEnv::getEnvConfig('files.image-mime-types'); + $mime_type = $this->getMimeType(); + return idx($mime_map, $mime_type); + } + public function isTransformableImage() { // NOTE: The way the 'gd' extension works in PHP is that you can install it diff --git a/src/applications/files/storage/file/__init__.php b/src/applications/files/storage/file/__init__.php index 9517290ec1..4de0139077 100644 --- a/src/applications/files/storage/file/__init__.php +++ b/src/applications/files/storage/file/__init__.php @@ -11,6 +11,7 @@ phutil_require_module('phabricator', 'applications/files/storage/base'); phutil_require_module('phabricator', 'applications/phid/constants'); phutil_require_module('phabricator', 'applications/phid/storage/phid'); phutil_require_module('phabricator', 'infrastructure/env'); +phutil_require_module('phabricator', 'infrastructure/util/hash'); phutil_require_module('phutil', 'error'); phutil_require_module('phutil', 'filesystem'); diff --git a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php b/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php index 9468281a0b..c6a2c007a1 100644 --- a/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php +++ b/src/storage/connection/mysql/AphrontMySQLDatabaseConnection.php @@ -315,7 +315,7 @@ final class AphrontMySQLDatabaseConnection extends AphrontDatabaseConnection { case 1143: // Access denied to column throw new AphrontQueryAccessDeniedException($exmsg); case 1146: // No such table - case 1154: // Unknown column "..." in field list + case 1054: // Unknown column "..." in field list throw new AphrontQuerySchemaException($exmsg); default: // TODO: 1064 is syntax error, and quite terrible in production.