Tokenize external editor links so they can be safely materialized on the client

Summary:
Ref T13515. Currently, opening a file to a particular line in an external editor relies on replacing "%l" with "%l" (which is escaped as "%25l") on the server, and then replacing "%25l" with the line number on the client. This will fail if the file path (or any other variable) contains "%l" in its unencoded form.

The parser also can't identify invalid variables.

Pull the parser out, formalize it, and make it generate an intermediate representation which can be sent to the client and reconstituted.

(This temporarily breaks Diffusion and permanently removes the weird, ancient integration in Dark Console.)

Test Plan:
  - Added a bunch of tests for the actual parser.
  - Used "Open in Editor" in Differential.

Maniphest Tasks: T13515

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

src/__phutil_library_map__.php

@@ -3275,6 +3275,9 @@ phutil_register_library_map(array(
     'PhabricatorEditorExtensionModule' => 'applications/transactions/engineextension/PhabricatorEditorExtensionModule.php',
     'PhabricatorEditorMailEngineExtension' => 'applications/transactions/engineextension/PhabricatorEditorMailEngineExtension.php',
     'PhabricatorEditorSetting' => 'applications/settings/setting/PhabricatorEditorSetting.php',
+    'PhabricatorEditorURIEngine' => 'infrastructure/editor/PhabricatorEditorURIEngine.php',
+    'PhabricatorEditorURIEngineTestCase' => 'infrastructure/editor/__tests__/PhabricatorEditorURIEngineTestCase.php',
+    'PhabricatorEditorURIParserException' => 'infrastructure/editor/PhabricatorEditorURIParserException.php',
     'PhabricatorElasticFulltextStorageEngine' => 'applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php',
     'PhabricatorElasticsearchHost' => 'infrastructure/cluster/search/PhabricatorElasticsearchHost.php',
     'PhabricatorElasticsearchQueryBuilder' => 'applications/search/fulltextstorage/PhabricatorElasticsearchQueryBuilder.php',
@@ -3543,7 +3546,6 @@ phutil_register_library_map(array(
     'PhabricatorHelpApplication' => 'applications/help/application/PhabricatorHelpApplication.php',
     'PhabricatorHelpController' => 'applications/help/controller/PhabricatorHelpController.php',
     'PhabricatorHelpDocumentationController' => 'applications/help/controller/PhabricatorHelpDocumentationController.php',
-    'PhabricatorHelpEditorProtocolController' => 'applications/help/controller/PhabricatorHelpEditorProtocolController.php',
     'PhabricatorHelpKeyboardShortcutController' => 'applications/help/controller/PhabricatorHelpKeyboardShortcutController.php',
     'PhabricatorHeraldApplication' => 'applications/herald/application/PhabricatorHeraldApplication.php',
     'PhabricatorHeraldContentSource' => 'applications/herald/contentsource/PhabricatorHeraldContentSource.php',
@@ -9735,6 +9737,9 @@ phutil_register_library_map(array(
     'PhabricatorEditorExtensionModule' => 'PhabricatorConfigModule',
     'PhabricatorEditorMailEngineExtension' => 'PhabricatorMailEngineExtension',
     'PhabricatorEditorSetting' => 'PhabricatorStringSetting',
+    'PhabricatorEditorURIEngine' => 'Phobject',
+    'PhabricatorEditorURIEngineTestCase' => 'PhabricatorTestCase',
+    'PhabricatorEditorURIParserException' => 'Exception',
     'PhabricatorElasticFulltextStorageEngine' => 'PhabricatorFulltextStorageEngine',
     'PhabricatorElasticsearchHost' => 'PhabricatorSearchHost',
     'PhabricatorElasticsearchSetupCheck' => 'PhabricatorSetupCheck',
@@ -10054,7 +10059,6 @@ phutil_register_library_map(array(
     'PhabricatorHelpApplication' => 'PhabricatorApplication',
     'PhabricatorHelpController' => 'PhabricatorController',
     'PhabricatorHelpDocumentationController' => 'PhabricatorHelpController',
-    'PhabricatorHelpEditorProtocolController' => 'PhabricatorHelpController',
     'PhabricatorHelpKeyboardShortcutController' => 'PhabricatorHelpController',
     'PhabricatorHeraldApplication' => 'PhabricatorApplication',
     'PhabricatorHeraldContentSource' => 'PhabricatorContentSource',

src/applications/console/plugin/DarkConsoleErrorLogPlugin.php

@@ -65,20 +65,9 @@ final class DarkConsoleErrorLogPlugin extends DarkConsolePlugin {
         $href = null;
         if (isset($entry['file'])) {
           $line .= ' called at ['.$entry['file'].':'.$entry['line'].']';
-          try {
-            $user = $this->getRequest()->getUser();
-            $href = $user->loadEditorLink($entry['file'], $entry['line'], null);
-          } catch (Exception $ex) {
-            // The database can be inaccessible.
-          }
-        }
 
-        $details[] = phutil_tag(
-          'a',
-          array(
-            'href' => $href,
-          ),
-          $line);
+        }
+        $details[] = $line;
         $details[] = "\n";
       }
 

src/applications/differential/view/DifferentialChangesetDetailView.php

@@ -252,17 +252,20 @@ final class DifferentialChangesetDetailView extends AphrontView {
   }
 
   private function getEditorURI() {
-    $viewer = $this->getViewer();
-
-    if (!$viewer->isLoggedIn()) {
-      return null;
-    }
-
     $repository = $this->getRepository();
     if (!$repository) {
       return null;
     }
 
+    $viewer = $this->getViewer();
+
+    $link_engine = PhabricatorEditorURIEngine::newForViewer($viewer);
+    if (!$link_engine) {
+      return null;
+    }
+
+    $link_engine->setRepository($repository);
+
     $changeset = $this->getChangeset();
     $diff = $this->getDiff();
 
@@ -271,7 +274,7 @@ final class DifferentialChangesetDetailView extends AphrontView {
 
     $line = idx($changeset->getMetadata(), 'line:first', 1);
 
-    return $viewer->loadEditorLink($path, $line, $repository);
+    return $link_engine->getURIForPath($path, $line);
   }
 
   private function getEditorConfigureURI() {

src/applications/diffusion/controller/DiffusionBrowseController.php

@@ -478,8 +478,9 @@ final class DiffusionBrowseController extends DiffusionController {
     $line = nonempty((int)$drequest->getLine(), 1);
     $buttons = array();
 
-    $editor_link = $user->loadEditorLink($path, $line, $repository);
-    $template = $user->loadEditorLink($path, '%l', $repository);
+    // TODO: Restore these behaviors.
+    $editor_link = null;
+    $template = null;
 
     $buttons[] =
       id(new PHUIButtonView())

src/applications/help/application/PhabricatorHelpApplication.php

@@ -18,7 +18,6 @@ final class PhabricatorHelpApplication extends PhabricatorApplication {
     return array(
       '/help/' => array(
         'keyboardshortcut/' => 'PhabricatorHelpKeyboardShortcutController',
-        'editorprotocol/' => 'PhabricatorHelpEditorProtocolController',
         'documentation/(?P<application>\w+)/'
           => 'PhabricatorHelpDocumentationController',
       ),

src/applications/help/controller/PhabricatorHelpEditorProtocolController.php

@@ -1,47 +0,0 @@
-<?php
-
-final class PhabricatorHelpEditorProtocolController
-  extends PhabricatorHelpController {
-
-  public function shouldAllowPublic() {
-    return true;
-  }
-
-  public function handleRequest(AphrontRequest $request) {
-    $viewer = $request->getViewer();
-
-    return $this->newDialog()
-      ->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('/');
-  }
-
-  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

@@ -471,41 +471,6 @@ final class PhabricatorUser
     return $this->getUserSetting(PhabricatorPronounSetting::SETTINGKEY);
   }
 
-  public function loadEditorLink(
-    $path,
-    $line,
-    PhabricatorRepository $repository = null) {
-
-    $editor = $this->getUserSetting(PhabricatorEditorSetting::SETTINGKEY);
-
-    if (!strlen($editor)) {
-      return null;
-    }
-
-    if ($repository) {
-      $callsign = $repository->getCallsign();
-    } else {
-      $callsign = null;
-    }
-
-    $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;
-  }
-
   /**
    * Populate the nametoken table, which used to fetch typeahead results. When
    * a user types "linc", we want to match "Abraham Lincoln" from on-demand

src/applications/settings/setting/PhabricatorEditorSetting.php

@@ -43,26 +43,9 @@ final class PhabricatorEditorSetting
       return;
     }
 
-    $ok = PhabricatorHelpEditorProtocolController::hasAllowedProtocol($value);
-    if ($ok) {
-      return;
-    }
-
-    $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.'://';
-    }
-
-    throw new Exception(
-      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" in Config.',
-        implode(', ', $proto_names),
-        $allowed_key));
+    id(new PhabricatorEditorURIEngine())
+      ->setPattern($value)
+      ->validatePattern();
   }
 
 }

src/infrastructure/editor/PhabricatorEditorURIEngine.php

@@ -0,0 +1,335 @@
+<?php
+
+final class PhabricatorEditorURIEngine
+  extends Phobject {
+
+  private $viewer;
+  private $repository;
+  private $pattern;
+  private $rawTokens;
+  private $repositoryTokens;
+
+  public static function newForViewer(PhabricatorUser $viewer) {
+    if (!$viewer->isLoggedIn()) {
+      return null;
+    }
+
+    $pattern = $viewer->getUserSetting(PhabricatorEditorSetting::SETTINGKEY);
+
+    if (!strlen(trim($pattern))) {
+      return null;
+    }
+
+    $engine = id(new self())
+      ->setViewer($viewer)
+      ->setPattern($pattern);
+
+    // If there's a problem with the pattern,
+
+    try {
+      $engine->validatePattern();
+    } catch (PhabricatorEditorURIParserException $ex) {
+      return null;
+    }
+
+    return $engine;
+  }
+
+  public function setViewer(PhabricatorUser $viewer) {
+    $this->viewer = $viewer;
+    return $this;
+  }
+
+  public function getViewer() {
+    return $this->viewer;
+  }
+
+  public function setRepository(PhabricatorRepository $repository) {
+    $this->repository = $repository;
+    return $this;
+  }
+
+  public function getRepository() {
+    return $this->repository;
+  }
+
+  public function setPattern($pattern) {
+    $this->pattern = $pattern;
+    return $this;
+  }
+
+  public function getPattern() {
+    return $this->pattern;
+  }
+
+  public function validatePattern() {
+    $this->getRawURITokens();
+    return true;
+  }
+
+  public function getURIForPath($path, $line) {
+    $tokens = $this->getURITokensForRepository();
+
+    $variables = array(
+      'f' => $this->escapeToken($path),
+      'l' => $this->escapeToken($line),
+    );
+
+    $tokens = $this->newTokensWithVariables($tokens, $variables);
+
+    return $this->newStringFromTokens($tokens);
+  }
+
+  public function getURITokensForRepository() {
+    if (!$this->repositoryTokens) {
+      $this->repositoryTokens = $this->newURITokensForRepository();
+    }
+
+    return $this->repositoryTokens;
+  }
+
+  public static function getVariableDefinitions() {
+    return array(
+      '%' => array(
+        'name' => pht('Literal Percent Symbol'),
+      ),
+      'r' => array(
+        'name' => pht('Repository Callsign'),
+      ),
+      'f' => array(
+        'name' => pht('File Name'),
+      ),
+      'l' => array(
+        'name' => pht('Line Number'),
+      ),
+    );
+  }
+
+  private function newURITokensForRepository() {
+    $tokens = $this->getRawURITokens();
+
+    $repository = $this->getRepository();
+    if (!$repository) {
+      throw new PhutilInvalidStateException('setRepository');
+    }
+
+    $variables = array(
+      'r' => $this->escapeToken($repository->getCallsign()),
+    );
+
+    return $this->newTokensWithVariables($tokens, $variables);
+  }
+
+  private function getRawURITokens() {
+    if (!$this->rawTokens) {
+      $this->rawTokens = $this->newRawURITokens();
+    }
+    return $this->rawTokens;
+  }
+
+  private function newRawURITokens() {
+    $raw_pattern = $this->getPattern();
+    $raw_tokens = self::newPatternTokens($raw_pattern);
+
+    $variable_definitions = self::getVariableDefinitions();
+
+    foreach ($raw_tokens as $token) {
+      if ($token['type'] !== 'variable') {
+        continue;
+      }
+
+      $value = $token['value'];
+
+      if (isset($variable_definitions[$value])) {
+        continue;
+      }
+
+      throw new PhabricatorEditorURIParserException(
+        pht(
+          'Editor pattern "%s" is invalid: the pattern contains an '.
+          'unrecognized variable ("%s"). Use "%%%%" to encode a literal '.
+          'percent symbol.',
+          $raw_pattern,
+          '%'.$value));
+    }
+
+    $variables = array(
+      '%' => '%',
+    );
+
+    $tokens = $this->newTokensWithVariables($raw_tokens, $variables);
+
+    $first_literal = null;
+    if ($tokens) {
+      foreach ($tokens as $token) {
+        if ($token['type'] === 'literal') {
+          $first_literal = $token['value'];
+        }
+        break;
+      }
+
+      if ($first_literal === null) {
+        throw new PhabricatorEditorURIParserException(
+          pht(
+            'Editor pattern "%s" is invalid: the pattern must begin with '.
+            'a valid editor protocol, but begins with a variable. This is '.
+            'very sneaky and also very forbidden.',
+            $raw_pattern));
+      }
+    }
+
+    $uri = new PhutilURI($first_literal);
+    $editor_protocol = $uri->getProtocol();
+
+    if (!$editor_protocol) {
+      throw new PhabricatorEditorURIParserException(
+        pht(
+          'Editor pattern "%s" is invalid: the pattern must begin with '.
+          'a valid editor protocol, but does not begin with a recognized '.
+          'protocol string.',
+          $raw_pattern));
+    }
+
+    $allowed_key = 'uri.allowed-editor-protocols';
+    $allowed_protocols = PhabricatorEnv::getEnvConfig($allowed_key);
+    if (empty($allowed_protocols[$editor_protocol])) {
+      throw new PhabricatorEditorURIParserException(
+        pht(
+          'Editor pattern "%s" is invalid: the pattern must begin with '.
+          'a valid editor protocol, but the protocol "%s://" is not allowed.',
+          $raw_pattern,
+          $editor_protocol));
+    }
+
+    return $tokens;
+  }
+
+  private function newTokensWithVariables(array $tokens, array $variables) {
+    // Replace all "variable" tokens that we have replacements for with
+    // the literal value.
+    foreach ($tokens as $key => $token) {
+      $type = $token['type'];
+
+      if ($type == 'variable') {
+        $variable = $token['value'];
+        if (isset($variables[$variable])) {
+          $tokens[$key] = array(
+            'type' => 'literal',
+            'value' => $variables[$variable],
+          );
+        }
+      }
+    }
+
+    // Now, merge sequences of adjacent "literal" tokens into a single token.
+    $last_literal = null;
+    foreach ($tokens as $key => $token) {
+      $is_literal = ($token['type'] === 'literal');
+
+      if (!$is_literal) {
+        $last_literal = null;
+        continue;
+      }
+
+      if ($last_literal !== null) {
+        $tokens[$key]['value'] =
+          $tokens[$last_literal]['value'].$token['value'];
+        unset($tokens[$last_literal]);
+      }
+
+      $last_literal = $key;
+    }
+
+    return $tokens;
+  }
+
+  private function escapeToken($token) {
+    // Paths are user controlled, so a clever user could potentially make
+    // editor links do surprising things with paths containing "/../".
+
+    // Find anything that looks like "/../" and mangle it.
+
+    $token = preg_replace('((^|/)\.\.(/|\z))', '\1dot-dot\2', $token);
+
+    return phutil_escape_uri($token);
+  }
+
+  private function newStringFromTokens(array $tokens) {
+    $result = array();
+
+    foreach ($tokens as $token) {
+      $token_type = $token['type'];
+      $token_value = $token['value'];
+
+      $is_literal = ($token_type === 'literal');
+      if (!$is_literal) {
+        throw new Exception(
+          pht(
+            'Editor pattern token list can not be converted into a string: '.
+            'it still contains a non-literal token ("%s", of type "%s").',
+            $token_value,
+            $token_type));
+      }
+
+      $result[] = $token_value;
+    }
+
+    $result = implode('', $result);
+
+    return $result;
+  }
+
+  public static function newPatternTokens($raw_pattern) {
+    $token_positions = array();
+
+    $len = strlen($raw_pattern);
+
+    for ($ii = 0; $ii < $len; $ii++) {
+      $c = $raw_pattern[$ii];
+      if ($c === '%') {
+        if (!isset($raw_pattern[$ii + 1])) {
+          throw new PhabricatorEditorURIParserException(
+            pht(
+              'Editor pattern "%s" is invalid: the final character in a '.
+              'pattern may not be an unencoded percent symbol ("%%"). '.
+              'Use "%%%%" to encode a literal percent symbol.',
+              $raw_pattern));
+        }
+
+        $token_positions[] = $ii;
+        $ii++;
+      }
+    }
+
+    // Add a final marker past the end of the string, so we'll collect any
+    // trailing literal bytes.
+    $token_positions[] = $len;
+
+    $tokens = array();
+    $cursor = 0;
+    foreach ($token_positions as $pos) {
+      $token_len = ($pos - $cursor);
+
+      if ($token_len > 0) {
+        $tokens[] = array(
+          'type' => 'literal',
+          'value' => substr($raw_pattern, $cursor, $token_len),
+        );
+      }
+
+      $cursor = $pos;
+
+      if ($cursor < $len) {
+        $tokens[] = array(
+          'type' => 'variable',
+          'value' => substr($raw_pattern, $cursor + 1, 1),
+        );
+      }
+
+      $cursor = $pos + 2;
+    }
+
+    return $tokens;
+  }
+
+}

src/infrastructure/editor/PhabricatorEditorURIParserException.php

@@ -0,0 +1,4 @@
+<?php
+
+final class PhabricatorEditorURIParserException
+  extends Exception {}

src/infrastructure/editor/__tests__/PhabricatorEditorURIEngineTestCase.php

@@ -0,0 +1,132 @@
+<?php
+
+final class PhabricatorEditorURIEngineTestCase
+  extends PhabricatorTestCase {
+
+  public function testPatternParsing() {
+    $map = array(
+      '' => array(),
+      '%' => false,
+      'aaa%' => false,
+      'quack' => array(
+        array(
+          'type' => 'literal',
+          'value' => 'quack',
+        ),
+      ),
+      '%a' => array(
+        array(
+          'type' => 'variable',
+          'value' => 'a',
+        ),
+      ),
+      '%%' => array(
+        array(
+          'type' => 'variable',
+          'value' => '%',
+        ),
+      ),
+      'x%y' => array(
+        array(
+          'type' => 'literal',
+          'value' => 'x',
+        ),
+        array(
+          'type' => 'variable',
+          'value' => 'y',
+        ),
+      ),
+      '%xy' => array(
+        array(
+          'type' => 'variable',
+          'value' => 'x',
+        ),
+        array(
+          'type' => 'literal',
+          'value' => 'y',
+        ),
+      ),
+      'x%yz' => array(
+        array(
+          'type' => 'literal',
+          'value' => 'x',
+        ),
+        array(
+          'type' => 'variable',
+          'value' => 'y',
+        ),
+        array(
+          'type' => 'literal',
+          'value' => 'z',
+        ),
+      ),
+    );
+
+    foreach ($map as $input => $expect) {
+      try {
+        $actual = PhabricatorEditorURIEngine::newPatternTokens($input);
+      } catch (Exception $ex) {
+        if ($expect !== false) {
+          throw $ex;
+        }
+        $actual = false;
+      }
+
+      $this->assertEqual(
+        $expect,
+        $actual,
+        pht('Parse of: %s', $input));
+    }
+  }
+
+  public function testPatternProtocols() {
+    $protocols = array(
+      'xyz',
+    );
+    $protocols = array_fuse($protocols);
+
+    $env = PhabricatorEnv::beginScopedEnv();
+    $env->overrideEnvConfig('uri.allowed-editor-protocols', $protocols);
+
+    $map = array(
+      'xyz:' => true,
+      'xyz:%%' => true,
+      'xyz://a' => true,
+      'xyz://open/?file=%f' => true,
+
+      '' => false,
+      '%%' => false,
+      '%r' => false,
+      'aaa' => false,
+      'xyz%%://' => false,
+      'http://' => false,
+
+      // These are fragments that "PhutilURI" can't figure out the protocol
+      // for. In theory, they would be safe to allow, they just probably are
+      // not very useful.
+
+      'xyz://' => false,
+      'xyz://%%' => false,
+    );
+
+    foreach ($map as $input => $expect) {
+      try {
+        id(new PhabricatorEditorURIEngine())
+          ->setPattern($input)
+          ->validatePattern();
+
+        $actual = true;
+      } catch (PhabricatorEditorURIParserException $ex) {
+        $actual = false;
+      }
+
+      $this->assertEqual(
+        $expect,
+        $actual,
+        pht(
+          'Allowed editor "xyz://" template: %s.',
+          $input));
+    }
+  }
+
+}