Whitelist allowed editor protocols

Summary:
This is the other half of D8548. Specifically, the attack here was to set your own editor link to `javascript\n:...` and then you could XSS yourself. This isn't a hugely damaging attack, but we can be more certain by adding a whitelist here.

We already whitelist linkable protocols in remarkup (`uri.allowed-protocols`) in general.

Test Plan:
Tried to set and use valid/invalid editor URIs.

{F130883}

{F130884}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

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

src/__phutil_library_map__.php

@@ -1090,6 +1090,7 @@ phutil_register_library_map(array(
     'PhabricatorApplicationFiles' => 'applications/files/application/PhabricatorApplicationFiles.php',
     'PhabricatorApplicationFlags' => 'applications/flag/application/PhabricatorApplicationFlags.php',
     'PhabricatorApplicationHarbormaster' => 'applications/harbormaster/application/PhabricatorApplicationHarbormaster.php',
+    'PhabricatorApplicationHelp' => 'applications/help/application/PhabricatorApplicationHelp.php',
     'PhabricatorApplicationHerald' => 'applications/herald/application/PhabricatorApplicationHerald.php',
     'PhabricatorApplicationHome' => 'applications/home/application/PhabricatorApplicationHome.php',
     'PhabricatorApplicationLaunchView' => 'applications/meta/view/PhabricatorApplicationLaunchView.php',
@@ -1129,6 +1130,7 @@ phutil_register_library_map(array(
     'PhabricatorApplicationSlowvote' => 'applications/slowvote/application/PhabricatorApplicationSlowvote.php',
     'PhabricatorApplicationStatusView' => 'applications/meta/view/PhabricatorApplicationStatusView.php',
     'PhabricatorApplicationSubscriptions' => 'applications/subscriptions/application/PhabricatorApplicationSubscriptions.php',
+    'PhabricatorApplicationSupport' => 'applications/support/application/PhabricatorApplicationSupport.php',
     'PhabricatorApplicationSystem' => 'applications/system/application/PhabricatorApplicationSystem.php',
     'PhabricatorApplicationTest' => 'applications/base/controller/__tests__/PhabricatorApplicationTest.php',
     'PhabricatorApplicationTokens' => 'applications/tokens/application/PhabricatorApplicationTokens.php',
@@ -1569,6 +1571,7 @@ phutil_register_library_map(array(
     'PhabricatorHash' => 'infrastructure/util/PhabricatorHash.php',
     'PhabricatorHashTestCase' => 'infrastructure/util/__tests__/PhabricatorHashTestCase.php',
     'PhabricatorHelpController' => 'applications/help/controller/PhabricatorHelpController.php',
+    'PhabricatorHelpEditorProtocolController' => 'applications/help/controller/PhabricatorHelpEditorProtocolController.php',
     'PhabricatorHelpKeyboardShortcutController' => 'applications/help/controller/PhabricatorHelpKeyboardShortcutController.php',
     'PhabricatorHomeController' => 'applications/home/controller/PhabricatorHomeController.php',
     'PhabricatorHomeMainController' => 'applications/home/controller/PhabricatorHomeMainController.php',
@@ -3751,6 +3754,7 @@ phutil_register_library_map(array(
     'PhabricatorApplicationFiles' => 'PhabricatorApplication',
     'PhabricatorApplicationFlags' => 'PhabricatorApplication',
     'PhabricatorApplicationHarbormaster' => 'PhabricatorApplication',
+    'PhabricatorApplicationHelp' => 'PhabricatorApplication',
     'PhabricatorApplicationHerald' => 'PhabricatorApplication',
     'PhabricatorApplicationHome' => 'PhabricatorApplication',
     'PhabricatorApplicationLaunchView' => 'AphrontView',
@@ -3788,6 +3792,7 @@ phutil_register_library_map(array(
     'PhabricatorApplicationSlowvote' => 'PhabricatorApplication',
     'PhabricatorApplicationStatusView' => 'AphrontView',
     'PhabricatorApplicationSubscriptions' => 'PhabricatorApplication',
+    'PhabricatorApplicationSupport' => 'PhabricatorApplication',
     'PhabricatorApplicationSystem' => 'PhabricatorApplication',
     'PhabricatorApplicationTest' => 'PhabricatorApplication',
     'PhabricatorApplicationTokens' => 'PhabricatorApplication',
@@ -4315,6 +4320,7 @@ phutil_register_library_map(array(
     'PhabricatorHarbormasterConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorHashTestCase' => 'PhabricatorTestCase',
     'PhabricatorHelpController' => 'PhabricatorController',
+    'PhabricatorHelpEditorProtocolController' => 'PhabricatorHelpController',
     'PhabricatorHelpKeyboardShortcutController' => 'PhabricatorHelpController',
     'PhabricatorHomeController' => 'PhabricatorController',
     'PhabricatorHomeMainController' => 'PhabricatorHomeController',

src/aphront/configuration/AphrontDefaultApplicationConfiguration.php

@@ -23,9 +23,6 @@ class AphrontDefaultApplicationConfiguration
         '' => 'DarkConsoleController',
         'data/(?P<key>[^/]+)/' => 'DarkConsoleDataController',
       ),
-      '/help/' => array(
-        'keyboardshortcut/' => 'PhabricatorHelpKeyboardShortcutController',
-      ),
     );
   }
 

src/applications/config/option/PhabricatorSecurityConfigOptions.php

@@ -12,6 +12,9 @@ final class PhabricatorSecurityConfigOptions
   }
 
   public function getOptions() {
+    $support_href = PhabricatorEnv::getDoclink(
+      'article/feedback.html');
+
     return array(
       $this->newOption('security.alternate-file-domain', 'string', null)
         ->setSummary(pht("Alternate domain to serve files from."))
@@ -126,6 +129,41 @@ final class PhabricatorSecurityConfigOptions
         ->addExample(
           '{"http": true, "https": true"}', pht('Valid Setting'))
         ->setLocked(true),
+      $this->newOption(
+        'uri.allowed-editor-protocols',
+        'set',
+        array(
+          'http' => true,
+          'https' => true,
+
+          // This handler is installed by Textmate.
+          'txmt' => true,
+
+          // This handler is for MacVim.
+          'mvim' => true,
+
+          // Unofficial handler for Vim.
+          'vim' => true,
+
+          // Unofficial handler for Sublime.
+          'subl' => true,
+
+          // Unofficial handler for Emacs.
+          'emacs' => true,
+
+          // This isn't a standard handler installed by an application, but
+          // is a reasonable name for a user-installed handler.
+          'editor' => true,
+        ))
+        ->setSummary(pht('Whitelists editor protocols for "Open in Editor".'))
+        ->setDescription(
+          pht(
+            "Users can configure a URI pattern to open files in a text ".
+            "editor. The URI must use a protocol on this whitelist.\n\n".
+            "(If you use an editor which defines a protocol not on this ".
+            "list, [[ %s | let us know ]] and we'll update the defaults.)",
+            $support_href))
+        ->setLocked(true),
        $this->newOption(
          'celerity.resource-hash',
          'string',

src/applications/help/application/PhabricatorApplicationHelp.php

@@ -0,0 +1,22 @@
+<?php
+
+final class PhabricatorApplicationHelp extends PhabricatorApplication {
+
+  public function canUninstall() {
+    return false;
+  }
+
+  public function isUnlisted() {
+    return true;
+  }
+
+  public function getRoutes() {
+    return array(
+      '/help/' => array(
+        'keyboardshortcut/' => 'PhabricatorHelpKeyboardShortcutController',
+        'editorprotocol/' => 'PhabricatorHelpEditorProtocolController',
+      ),
+    );
+  }
+
+}

src/applications/help/controller/PhabricatorHelpEditorProtocolController.php

@@ -0,0 +1,52 @@
+<?php
+
+final class PhabricatorHelpEditorProtocolController
+  extends PhabricatorHelpController {
+
+  public function shouldAllowPublic() {
+    return true;
+  }
+
+  public function processRequest() {
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+
+    $dialog = id(new AphrontDialogView())
+      ->setUser($viewer)
+      ->setMethod('GET')
+      ->setSubmitURI('/settings/panel/display/')
+      ->setTitle(pht('Unsupported Editor Protocol'))
+      ->appendParagraph(
+        pht(
+          'Your configured editor URI uses an unsupported protocol. Change '.
+          'your settings to use a supported protocol, or ask your '.
+          'administrator to add support for the chosen protocol by '.
+          'configuring: %s',
+          phutil_tag('tt', array(), 'uri.allowed-editor-protocols')))
+      ->addSubmitButton(pht('Change Settings'))
+      ->addCancelButton('/');
+
+    return id(new AphrontDialogResponse())
+      ->setDialog($dialog);
+  }
+
+  public static function hasAllowedProtocol($uri) {
+    $uri = new PhutilURI($uri);
+    $editor_protocol = $uri->getProtocol();
+    if (!$editor_protocol) {
+      // The URI must have a protocol.
+      return false;
+    }
+
+    $allowed_key = 'uri.allowed-editor-protocols';
+    $allowed_protocols = PhabricatorEnv::getEnvConfig($allowed_key);
+    if (empty($allowed_protocols[$editor_protocol])) {
+      // The protocol must be on the allowed protocol whitelist.
+      return false;
+    }
+
+    return true;
+  }
+
+
+}

src/applications/people/storage/PhabricatorUser.php

@@ -441,14 +441,26 @@ final class PhabricatorUser
       }
     }
 
-    if ($editor) {
+    if (!strlen($editor)) {
-      return strtr($editor, array(
+      return null;
-        '%%' => '%',
-        '%f' => phutil_escape_uri($path),
-        '%l' => phutil_escape_uri($line),
-        '%r' => phutil_escape_uri($callsign),
-      ));
     }
+
+    $uri = strtr($editor, array(
+      '%%' => '%',
+      '%f' => phutil_escape_uri($path),
+      '%l' => phutil_escape_uri($line),
+      '%r' => phutil_escape_uri($callsign),
+    ));
+
+    // The resulting URI must have an allowed protocol. Otherwise, we'll return
+    // a link to an error page explaining the misconfiguration.
+
+    $ok = PhabricatorHelpEditorProtocolController::hasAllowedProtocol($uri);
+    if (!$ok) {
+      return '/help/editorprotocol/';
+    }
+
+    return (string)$uri;
   }
 
   public function getAlternateCSRFString() {

src/applications/settings/panel/PhabricatorSettingsPanelDisplayPreferences.php

@@ -26,6 +26,8 @@ final class PhabricatorSettingsPanelDisplayPreferences
     $pref_monospaced_textareas =
       PhabricatorUserPreferences::PREFERENCE_MONOSPACED_TEXTAREAS;
 
+    $errors = array();
+    $e_editor = null;
     if ($request->isFormPost()) {
       $monospaced = $request->getStr($pref_monospaced);
 
@@ -42,9 +44,35 @@ final class PhabricatorSettingsPanelDisplayPreferences
         $pref_monospaced_textareas,
         $request->getStr($pref_monospaced_textareas));
 
-      $preferences->save();
+      $editor_pattern = $preferences->getPreference($pref_editor);
-      return id(new AphrontRedirectResponse())
+      if (strlen($editor_pattern)) {
-        ->setURI($this->getPanelURI('?saved=true'));
+        $ok = PhabricatorHelpEditorProtocolController::hasAllowedProtocol(
+          $editor_pattern);
+        if (!$ok) {
+          $allowed_key = 'uri.allowed-editor-protocols';
+          $allowed_protocols = PhabricatorEnv::getEnvConfig($allowed_key);
+
+          $proto_names = array();
+          foreach (array_keys($allowed_protocols) as $protocol) {
+            $proto_names[] = $protocol.'://';
+          }
+
+          $errors[] = pht(
+            'Editor link has an invalid or missing protocol. You must '.
+            'use a whitelisted editor protocol from this list: %s. To '.
+            'add protocols, update %s.',
+            implode(', ', $proto_names),
+            phutil_tag('tt', array(), $allowed_key));
+
+          $e_editor = pht('Invalid');
+        }
+      }
+
+      if (!$errors) {
+        $preferences->save();
+        return id(new AphrontRedirectResponse())
+          ->setURI($this->getPanelURI('?saved=true'));
+      }
     }
 
     $example_string = <<<EXAMPLE
@@ -95,8 +123,8 @@ EXAMPLE;
         id(new AphrontFormTextControl())
         ->setLabel(pht('Editor Link'))
         ->setName($pref_editor)
-        // How to pht()
         ->setCaption($editor_instructions)
+        ->setError($e_editor)
         ->setValue($preferences->getPreference($pref_editor)))
       ->appendChild(
         id(new AphrontFormSelectControl())
@@ -139,6 +167,7 @@ EXAMPLE;
 
     $form_box = id(new PHUIObjectBoxView())
       ->setHeaderText(pht('Display Preferences'))
+      ->setFormErrors($errors)
       ->setFormSaved($request->getStr('saved') === 'true')
       ->setForm($form);
 

src/applications/support/application/PhabricatorApplicationSupport.php

@@ -0,0 +1,21 @@
+<?php
+
+final class PhabricatorApplicationSupport extends PhabricatorApplication {
+
+  public function canUninstall() {
+    return false;
+  }
+
+  public function isUnlisted() {
+    return true;
+  }
+
+  public function getRoutes() {
+    return array(
+      '/help/' => array(
+        'keyboardshortcut/' => 'PhabricatorHelpKeyboardShortcutController',
+      ),
+    );
+  }
+
+}