From 03ebbccbc99fb47037ce84c0707d790af6f762c7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 2 May 2011 14:20:24 -0700 Subject: [PATCH] Restore image proxying to Remarkup Summary: Previously, Remarkup allowed you to paste in an image URI and get an inline image. However, it did this by hotlinking the image which isn't so hot in an open source product. Restore this feature, but use image proxying instead. The existing image macro code does most of the work. There is a mild security risk depending on the network setup so I've left this default-disabled and made a note about it. It should be safe to enable for Facebook. Test Plan: Pasted in image and non-image links, got reasonable behavior. Verified proxying appears to work. Verified that file:// shenanigans produce 400. Reviewed By: tuomaspelkonen Reviewers: aran, jungejason, tuomaspelkonen Commenters: cpiro CC: aran, cpiro, tuomaspelkonen Differential Revision: 214 --- conf/default.conf.php | 10 +++ resources/sql/patches/035.proxyimage.sql | 6 ++ src/__celerity_resource_map__.php | 34 +++++------ src/__phutil_library_map__.php | 6 ++ ...AphrontDefaultApplicationConfiguration.php | 1 + .../DifferentialMarkupEngineFactory.php | 3 + .../differential/parser/markup/__init__.php | 1 + .../proxy/PhabricatorFileProxyController.php | 55 +++++++++++++++++ .../files/controller/proxy/__init__.php | 20 ++++++ .../files/storage/file/PhabricatorFile.php | 11 ++++ .../proxyimage/PhabricatorFileProxyImage.php | 34 +++++++++++ .../files/storage/proxyimage/__init__.php | 14 +++++ .../PhabricatorRemarkupRuleProxyImage.php | 61 +++++++++++++++++++ .../markuprule/proxyimage/__init__.php | 15 +++++ webroot/rsrc/css/core/remarkup.css | 5 ++ 15 files changed, 259 insertions(+), 17 deletions(-) create mode 100644 resources/sql/patches/035.proxyimage.sql create mode 100644 src/applications/files/controller/proxy/PhabricatorFileProxyController.php create mode 100644 src/applications/files/controller/proxy/__init__.php create mode 100644 src/applications/files/storage/proxyimage/PhabricatorFileProxyImage.php create mode 100644 src/applications/files/storage/proxyimage/__init__.php create mode 100644 src/infrastructure/markup/remarkup/markuprule/proxyimage/PhabricatorRemarkupRuleProxyImage.php create mode 100644 src/infrastructure/markup/remarkup/markuprule/proxyimage/__init__.php diff --git a/conf/default.conf.php b/conf/default.conf.php index c8b6825124..000bdc5e56 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -292,6 +292,16 @@ return array( 'text/plain' => 'text/plain; charset=utf-8', ), + // 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 + // make HTTP requests to arbitrary URIs. If the server has access to internal + // resources, this could be a security risk. You should only enable it if you + // are installed entirely a VPN and VPN access is required to access + // Phabricator, or if the webserver has no special access to anything. If + // unsure, it is safer to leave this disabled. + 'files.enable-proxy' => false, + // -- Differential ---------------------------------------------------------- // 'differential.revision-custom-detail-renderer' => null, diff --git a/resources/sql/patches/035.proxyimage.sql b/resources/sql/patches/035.proxyimage.sql new file mode 100644 index 0000000000..b5533558d9 --- /dev/null +++ b/resources/sql/patches/035.proxyimage.sql @@ -0,0 +1,6 @@ +CREATE TABLE phabricator_file.file_proxyimage ( + id int unsigned not null primary key auto_increment, + uri varchar(255) binary not null, + unique key(uri), + filePHID varchar(64) binary not null +) ENGINE=InnoDB; diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 15f450663c..4fa9111eca 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -616,7 +616,7 @@ celerity_register_resource_map(array( ), 'phabricator-remarkup-css' => array( - 'uri' => '/res/1fc75ba6/rsrc/css/core/remarkup.css', + 'uri' => '/res/41748e59/rsrc/css/core/remarkup.css', 'type' => 'css', 'requires' => array( @@ -653,7 +653,7 @@ celerity_register_resource_map(array( ), array ( 'packages' => array ( - '14e8c91d' => + '4270730a' => array ( 'name' => 'core.pkg.css', 'symbols' => @@ -673,7 +673,7 @@ celerity_register_resource_map(array( 12 => 'phabricator-remarkup-css', 13 => 'syntax-highlighting-css', ), - 'uri' => '/res/pkg/14e8c91d/core.pkg.css', + 'uri' => '/res/pkg/4270730a/core.pkg.css', 'type' => 'css', ), '6c786373' => @@ -720,14 +720,14 @@ celerity_register_resource_map(array( ), 'reverse' => array ( - 'aphront-crumbs-view-css' => '14e8c91d', - 'aphront-dialog-view-css' => '14e8c91d', - 'aphront-form-view-css' => '14e8c91d', - 'aphront-panel-view-css' => '14e8c91d', - 'aphront-side-nav-view-css' => '14e8c91d', - 'aphront-table-view-css' => '14e8c91d', - 'aphront-tokenizer-control-css' => '14e8c91d', - 'aphront-typeahead-control-css' => '14e8c91d', + 'aphront-crumbs-view-css' => '4270730a', + 'aphront-dialog-view-css' => '4270730a', + 'aphront-form-view-css' => '4270730a', + 'aphront-panel-view-css' => '4270730a', + 'aphront-side-nav-view-css' => '4270730a', + 'aphront-table-view-css' => '4270730a', + 'aphront-tokenizer-control-css' => '4270730a', + 'aphront-typeahead-control-css' => '4270730a', 'differential-changeset-view-css' => '8d8a971a', 'differential-core-view-css' => '8d8a971a', 'differential-revision-add-comment-css' => '8d8a971a', @@ -742,11 +742,11 @@ celerity_register_resource_map(array( 'javelin-behavior-differential-feedback-preview' => '6c786373', 'javelin-behavior-differential-populate' => '6c786373', 'javelin-behavior-differential-show-more' => '6c786373', - 'phabricator-core-buttons-css' => '14e8c91d', - 'phabricator-core-css' => '14e8c91d', - 'phabricator-directory-css' => '14e8c91d', - 'phabricator-remarkup-css' => '14e8c91d', - 'phabricator-standard-page-view' => '14e8c91d', - 'syntax-highlighting-css' => '14e8c91d', + 'phabricator-core-buttons-css' => '4270730a', + 'phabricator-core-css' => '4270730a', + 'phabricator-directory-css' => '4270730a', + 'phabricator-remarkup-css' => '4270730a', + 'phabricator-standard-page-view' => '4270730a', + 'syntax-highlighting-css' => '4270730a', ), )); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 99834e2890..02ba6cd0ef 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -310,6 +310,8 @@ phutil_register_library_map(array( 'PhabricatorFileMacroDeleteController' => 'applications/files/controller/macrodelete', 'PhabricatorFileMacroEditController' => 'applications/files/controller/macroedit', 'PhabricatorFileMacroListController' => 'applications/files/controller/macrolist', + 'PhabricatorFileProxyController' => 'applications/files/controller/proxy', + 'PhabricatorFileProxyImage' => 'applications/files/storage/proxyimage', 'PhabricatorFileStorageBlob' => 'applications/files/storage/storageblob', 'PhabricatorFileURI' => 'applications/files/uri', 'PhabricatorFileUploadController' => 'applications/files/controller/upload', @@ -382,6 +384,7 @@ phutil_register_library_map(array( 'PhabricatorRemarkupRuleDiffusion' => 'infrastructure/markup/remarkup/markuprule/diffusion', 'PhabricatorRemarkupRuleImageMacro' => 'infrastructure/markup/remarkup/markuprule/imagemacro', 'PhabricatorRemarkupRuleManiphest' => 'infrastructure/markup/remarkup/markuprule/maniphest', + 'PhabricatorRemarkupRuleProxyImage' => 'infrastructure/markup/remarkup/markuprule/proxyimage', 'PhabricatorRepository' => 'applications/repository/storage/repository', 'PhabricatorRepositoryArcanistProject' => 'applications/repository/storage/arcanistproject', 'PhabricatorRepositoryArcanistProjectEditController' => 'applications/repository/controller/arcansistprojectedit', @@ -718,6 +721,8 @@ phutil_register_library_map(array( 'PhabricatorFileMacroDeleteController' => 'PhabricatorFileController', 'PhabricatorFileMacroEditController' => 'PhabricatorFileController', 'PhabricatorFileMacroListController' => 'PhabricatorFileController', + 'PhabricatorFileProxyController' => 'PhabricatorFileController', + 'PhabricatorFileProxyImage' => 'PhabricatorFileDAO', 'PhabricatorFileStorageBlob' => 'PhabricatorFileDAO', 'PhabricatorFileUploadController' => 'PhabricatorFileController', 'PhabricatorFileViewController' => 'PhabricatorFileController', @@ -782,6 +787,7 @@ phutil_register_library_map(array( 'PhabricatorRemarkupRuleDiffusion' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleImageMacro' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleManiphest' => 'PhutilRemarkupRule', + 'PhabricatorRemarkupRuleProxyImage' => 'PhutilRemarkupRule', 'PhabricatorRepository' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryArcanistProject' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryArcanistProjectEditController' => 'PhabricatorRepositoryController', diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php index bbca0aaa09..4aa8c1b68b 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -60,6 +60,7 @@ class AphrontDefaultApplicationConfiguration 'edit/(?:(?P\d+)/)?$' => 'PhabricatorFileMacroEditController', 'delete/(?P\d+)/$' => 'PhabricatorFileMacroDeleteController', ), + 'proxy/$' => 'PhabricatorFileProxyController', ), '/phid/' => array( '$' => 'PhabricatorPHIDLookupController', diff --git a/src/applications/differential/parser/markup/DifferentialMarkupEngineFactory.php b/src/applications/differential/parser/markup/DifferentialMarkupEngineFactory.php index a3f909298a..297b111000 100644 --- a/src/applications/differential/parser/markup/DifferentialMarkupEngineFactory.php +++ b/src/applications/differential/parser/markup/DifferentialMarkupEngineFactory.php @@ -28,6 +28,9 @@ class DifferentialMarkupEngineFactory { $rules = array(); $rules[] = new PhutilRemarkupRuleEscapeRemarkup(); + if (PhabricatorEnv::getEnvConfig('files.enable-proxy')) { + $rules[] = new PhabricatorRemarkupRuleProxyImage(); + } $rules[] = new PhutilRemarkupRuleHyperlink(); $rules[] = new PhabricatorRemarkupRuleDifferential(); diff --git a/src/applications/differential/parser/markup/__init__.php b/src/applications/differential/parser/markup/__init__.php index 87d1030636..2a4ccd6556 100644 --- a/src/applications/differential/parser/markup/__init__.php +++ b/src/applications/differential/parser/markup/__init__.php @@ -11,6 +11,7 @@ phutil_require_module('phabricator', 'infrastructure/markup/remarkup/markuprule/ phutil_require_module('phabricator', 'infrastructure/markup/remarkup/markuprule/diffusion'); phutil_require_module('phabricator', 'infrastructure/markup/remarkup/markuprule/imagemacro'); phutil_require_module('phabricator', 'infrastructure/markup/remarkup/markuprule/maniphest'); +phutil_require_module('phabricator', 'infrastructure/markup/remarkup/markuprule/proxyimage'); phutil_require_module('phutil', 'markup/engine/remarkup'); phutil_require_module('phutil', 'markup/engine/remarkup/blockrule/remarkupcode'); diff --git a/src/applications/files/controller/proxy/PhabricatorFileProxyController.php b/src/applications/files/controller/proxy/PhabricatorFileProxyController.php new file mode 100644 index 0000000000..3869376b12 --- /dev/null +++ b/src/applications/files/controller/proxy/PhabricatorFileProxyController.php @@ -0,0 +1,55 @@ +getRequest(); + $uri = $request->getStr('uri'); + + $proxy = id(new PhabricatorFileProxyImage())->loadOneWhere( + 'uri = %s', + $uri); + + if (!$proxy) { + $file = PhabricatorFile::newFromFileDownload( + $uri, + nonempty(basename($uri), 'proxied-file')); + if ($file) { + $proxy = new PhabricatorFileProxyImage(); + $proxy->setURI($uri); + $proxy->setFilePHID($file->getPHID()); + $proxy->save(); + } + } + + if ($proxy) { + $view_uri = PhabricatorFileURI::getViewURIForPHID($proxy->getFilePHID()); + return id(new AphrontRedirectResponse())->setURI($view_uri); + } + + return new Aphront400Response(); + } +} diff --git a/src/applications/files/controller/proxy/__init__.php b/src/applications/files/controller/proxy/__init__.php new file mode 100644 index 0000000000..f8c7b15690 --- /dev/null +++ b/src/applications/files/controller/proxy/__init__.php @@ -0,0 +1,20 @@ +getProtocol(); + switch ($protocol) { + case 'http': + case 'https': + break; + default: + // Make sure we are not accessing any file:// URIs or similar. + return null; + } + $timeout = stream_context_create( array( 'http' => array( diff --git a/src/applications/files/storage/proxyimage/PhabricatorFileProxyImage.php b/src/applications/files/storage/proxyimage/PhabricatorFileProxyImage.php new file mode 100644 index 0000000000..65c70ec62b --- /dev/null +++ b/src/applications/files/storage/proxyimage/PhabricatorFileProxyImage.php @@ -0,0 +1,34 @@ + false, + ) + parent::getConfiguration(); + } + + static public function getProxyImageURI($uri) { + return '/file/proxy/?uri='.phutil_escape_uri($uri); + } +} + diff --git a/src/applications/files/storage/proxyimage/__init__.php b/src/applications/files/storage/proxyimage/__init__.php new file mode 100644 index 0000000000..8e534a7cb3 --- /dev/null +++ b/src/applications/files/storage/proxyimage/__init__.php @@ -0,0 +1,14 @@ +]@', + array($this, 'markupProxyImage'), + $text); + + $text = preg_replace_callback( + '@(?<=^|\s)(\w{3,}://\S+'.$filetypes.')(?=\s|$)@', + array($this, 'markupProxyImage'), + $text); + + return $text; + } + + public function markupProxyImage($matches) { + + $uri = PhabricatorFileProxyImage::getProxyImageURI($matches[1]); + + return $this->getEngine()->storeText( + phutil_render_tag( + 'a', + array( + 'href' => $uri, + 'target' => '_blank', + ), + phutil_render_tag( + 'img', + array( + 'src' => $uri, + 'class' => 'remarkup-proxy-image', + )))); + } + +} diff --git a/src/infrastructure/markup/remarkup/markuprule/proxyimage/__init__.php b/src/infrastructure/markup/remarkup/markuprule/proxyimage/__init__.php new file mode 100644 index 0000000000..0036612593 --- /dev/null +++ b/src/infrastructure/markup/remarkup/markuprule/proxyimage/__init__.php @@ -0,0 +1,15 @@ +