Delete PhabricatorRemarkupRuleProxyImage

Summary: don't need it now that uploading files is so easy. Plus it made for some buggy jonx if / when there were bad image links coupled with caching. In theory this is a lot less pretty though if folks linked to a bunch of files served elsewhere using images.

Test Plan: http://does-not-exist.com/imaginary.jpg rendered as a link!

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2000

Differential Revision: https://secure.phabricator.com/D3908

conf/default.conf.php

@@ -856,17 +856,6 @@ return array(
     '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
-  // 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,
-
-
 // -- Storage --------------------------------------------------------------- //
 
   // Phabricator allows users to upload files, and can keep them in various

resources/sql/patches/dropfileproxyimage.sql

@@ -0,0 +1 @@
+DROP TABLE {$NAMESPACE}_file.file_proxyimage;

src/__phutil_library_map__.php

@@ -744,8 +744,6 @@ phutil_register_library_map(array(
     'PhabricatorFileLinkListView' => 'view/layout/PhabricatorFileLinkListView.php',
     'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php',
     'PhabricatorFileListController' => 'applications/files/controller/PhabricatorFileListController.php',
-    'PhabricatorFileProxyController' => 'applications/files/controller/PhabricatorFileProxyController.php',
-    'PhabricatorFileProxyImage' => 'applications/files/storage/PhabricatorFileProxyImage.php',
     'PhabricatorFileQuery' => 'applications/files/query/PhabricatorFileQuery.php',
     'PhabricatorFileShortcutController' => 'applications/files/controller/PhabricatorFileShortcutController.php',
     'PhabricatorFileSideNavView' => 'applications/files/view/PhabricatorFileSideNavView.php',
@@ -987,7 +985,6 @@ phutil_register_library_map(array(
     'PhabricatorRemarkupRuleObjectName' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleObjectName.php',
     'PhabricatorRemarkupRulePaste' => 'infrastructure/markup/rule/PhabricatorRemarkupRulePaste.php',
     'PhabricatorRemarkupRulePhriction' => 'infrastructure/markup/rule/PhabricatorRemarkupRulePhriction.php',
-    'PhabricatorRemarkupRuleProxyImage' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php',
     'PhabricatorRemarkupRuleYoutube' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleYoutube.php',
     'PhabricatorRepository' => 'applications/repository/storage/PhabricatorRepository.php',
     'PhabricatorRepositoryArcanistProject' => 'applications/repository/storage/PhabricatorRepositoryArcanistProject.php',
@@ -1955,8 +1952,6 @@ phutil_register_library_map(array(
     'PhabricatorFileLinkListView' => 'AphrontView',
     'PhabricatorFileLinkView' => 'AphrontView',
     'PhabricatorFileListController' => 'PhabricatorFileController',
-    'PhabricatorFileProxyController' => 'PhabricatorFileController',
-    'PhabricatorFileProxyImage' => 'PhabricatorFileDAO',
     'PhabricatorFileQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorFileShortcutController' => 'PhabricatorFileController',
     'PhabricatorFileSideNavView' => 'AphrontView',
@@ -2172,7 +2167,6 @@ phutil_register_library_map(array(
     'PhabricatorRemarkupRuleObjectName' => 'PhutilRemarkupRule',
     'PhabricatorRemarkupRulePaste' => 'PhabricatorRemarkupRuleObjectName',
     'PhabricatorRemarkupRulePhriction' => 'PhutilRemarkupRule',
-    'PhabricatorRemarkupRuleProxyImage' => 'PhutilRemarkupRule',
     'PhabricatorRemarkupRuleYoutube' => 'PhutilRemarkupRule',
     'PhabricatorRepository' => 'PhabricatorRepositoryDAO',
     'PhabricatorRepositoryArcanistProject' => 'PhabricatorRepositoryDAO',

src/applications/files/controller/PhabricatorFileProxyController.php

@@ -1,54 +0,0 @@
-<?php
-
-final class PhabricatorFileProxyController extends PhabricatorFileController {
-
-  private $uri;
-
-  public function processRequest() {
-
-    if (!PhabricatorEnv::getEnvConfig('files.enable-proxy')) {
-      return new Aphront400Response();
-    }
-
-    $request = $this->getRequest();
-    $uri = $request->getStr('uri');
-
-    $proxy = id(new PhabricatorFileProxyImage())->loadOneWhere(
-      'uri = %s',
-      $uri);
-
-    if (!$proxy) {
-      // This write is fine to skip CSRF checks for, we're just building a
-      // cache of some remote image.
-      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-
-      $file = PhabricatorFile::newFromFileDownload(
-        $uri,
-        nonempty(basename($uri), 'proxied-file'));
-      if ($file) {
-        $proxy = new PhabricatorFileProxyImage();
-        $proxy->setURI($uri);
-        $proxy->setFilePHID($file->getPHID());
-        $proxy->save();
-      }
-
-      unset($unguarded);
-    }
-
-    if ($proxy) {
-      $file = id(new PhabricatorFile())->loadOneWhere('phid = %s',
-                                                      $proxy->getFilePHID());
-      if ($file) {
-        $view_uri = $file->getBestURI();
-      } else {
-        $bad_phid = $proxy->getFilePHID();
-        throw new Exception(
-          "Unable to load file with phid {$bad_phid}."
-        );
-      }
-      return id(new AphrontRedirectResponse())->setURI($view_uri);
-    }
-
-    return new Aphront400Response();
-  }
-}

src/applications/files/storage/PhabricatorFileProxyImage.php

@@ -1,18 +0,0 @@
-<?php
-
-final class PhabricatorFileProxyImage extends PhabricatorFileDAO {
-
-  protected $uri;
-  protected $filePHID;
-
-  public function getConfiguration() {
-    return array(
-      self::CONFIG_TIMESTAMPS => false,
-    ) + parent::getConfiguration();
-  }
-
-  static public function getProxyImageURI($uri) {
-    return '/file/proxy/?uri='.phutil_escape_uri($uri);
-  }
-}
-

src/docs/userguide/remarkup.diviner

@@ -307,16 +307,14 @@ Valid options are:
 
 = Embedding Media =
 
-If you set configuration flags, you can embed media directly in text:
+If you set a configuration flag, you can embed media directly in text:
 
-  - **files.enable-proxy**: allows you to paste in image URLs and have them
-    render inline.
   - **remarkup.enable-embedded-youtube**: allows you to paste in YouTube videos
     and have them render inline.
 
-These options are disabled by default because they have security and/or
+This option is disabled by default because it has security and/or
-silliness implications, read their descriptions in ##default.conf.php## before
+silliness implications. Read the description in ##default.conf.php## before
-enabling them.
+enabling it.
 
 = Image Macros =
 

src/infrastructure/markup/PhabricatorMarkupEngine.php

@@ -41,7 +41,7 @@ final class PhabricatorMarkupEngine {
 
   private $objects = array();
   private $viewer;
-  private $version = 0;
+  private $version = 1;
 
 
 /* -(  Markup Pipeline  )---------------------------------------------------- */
@@ -286,7 +286,6 @@ final class PhabricatorMarkupEngine {
     return self::newMarkupEngine(
       array(
         'macros'      => false,
-        'fileproxy'   => false,
         'youtube'     => false,
 
       ));
@@ -345,7 +344,6 @@ final class PhabricatorMarkupEngine {
   private static function getMarkupEngineDefaultConfiguration() {
     return array(
       'pygments'      => PhabricatorEnv::getEnvConfig('pygments.enabled'),
-      'fileproxy'     => PhabricatorEnv::getEnvConfig('files.enable-proxy'),
       'youtube'       => PhabricatorEnv::getEnvConfig(
         'remarkup.enable-embedded-youtube'),
       'custom-inline' => array(),
@@ -394,10 +392,6 @@ final class PhabricatorMarkupEngine {
 
     $rules[] = new PhutilRemarkupRuleDocumentLink();
 
-    if ($options['fileproxy']) {
-      $rules[] = new PhabricatorRemarkupRuleProxyImage();
-    }
-
     if ($options['youtube']) {
       $rules[] = new PhabricatorRemarkupRuleYoutube();
     }

src/infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php

@@ -1,45 +0,0 @@
-<?php
-
-/**
- * @group markup
- */
-final class PhabricatorRemarkupRuleProxyImage
-  extends PhutilRemarkupRule {
-
-  public function apply($text) {
-
-    $filetypes = '\.(?:jpe?g|png|gif)';
-
-    $text = preg_replace_callback(
-      '@[<](\w{3,}://.+?'.$filetypes.')[>]@',
-      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',
-          ))));
-  }
-
-}

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

@@ -1020,6 +1020,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type'    => 'php',
         'name'    => $this->getPatchPath('liskcounters.php'),
       ),
+      'dropfileproxyimage.sql' => array(
+        'type'    => 'sql',
+        'name'    => $this->getPatchPath('dropfileproxyimage.sql'),
+      ),
     );
   }