From 3984c14260d3bce234ee9a57ffd7ff3e84cf599f Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 18 Apr 2020 11:31:27 -0700 Subject: [PATCH] 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 | 8 +- .../plugin/DarkConsoleErrorLogPlugin.php | 15 +- .../view/DifferentialChangesetDetailView.php | 17 +- .../controller/DiffusionBrowseController.php | 5 +- .../PhabricatorHelpApplication.php | 1 - ...habricatorHelpEditorProtocolController.php | 47 --- .../people/storage/PhabricatorUser.php | 35 -- .../setting/PhabricatorEditorSetting.php | 23 +- .../editor/PhabricatorEditorURIEngine.php | 335 ++++++++++++++++++ .../PhabricatorEditorURIParserException.php | 4 + .../PhabricatorEditorURIEngineTestCase.php | 132 +++++++ 11 files changed, 495 insertions(+), 127 deletions(-) delete mode 100644 src/applications/help/controller/PhabricatorHelpEditorProtocolController.php create mode 100644 src/infrastructure/editor/PhabricatorEditorURIEngine.php create mode 100644 src/infrastructure/editor/PhabricatorEditorURIParserException.php create mode 100644 src/infrastructure/editor/__tests__/PhabricatorEditorURIEngineTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ca5b8733f2..91e599dda7 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/console/plugin/DarkConsoleErrorLogPlugin.php b/src/applications/console/plugin/DarkConsoleErrorLogPlugin.php index 213b578c75..a95bfd6f0d 100644 --- a/src/applications/console/plugin/DarkConsoleErrorLogPlugin.php +++ b/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"; } diff --git a/src/applications/differential/view/DifferentialChangesetDetailView.php b/src/applications/differential/view/DifferentialChangesetDetailView.php index 0efb959969..a5d5fd1da4 100644 --- a/src/applications/differential/view/DifferentialChangesetDetailView.php +++ b/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() { diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 60df3f35ac..3cc873b360 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/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()) diff --git a/src/applications/help/application/PhabricatorHelpApplication.php b/src/applications/help/application/PhabricatorHelpApplication.php index deeae7fa94..6c40387956 100644 --- a/src/applications/help/application/PhabricatorHelpApplication.php +++ b/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\w+)/' => 'PhabricatorHelpDocumentationController', ), diff --git a/src/applications/help/controller/PhabricatorHelpEditorProtocolController.php b/src/applications/help/controller/PhabricatorHelpEditorProtocolController.php deleted file mode 100644 index 22ad0f8d2d..0000000000 --- a/src/applications/help/controller/PhabricatorHelpEditorProtocolController.php +++ /dev/null @@ -1,47 +0,0 @@ -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; - } - - -} diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 304c3f9506..fa6dc08e9f 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/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 diff --git a/src/applications/settings/setting/PhabricatorEditorSetting.php b/src/applications/settings/setting/PhabricatorEditorSetting.php index fcc121334b..fd1fae8e70 100644 --- a/src/applications/settings/setting/PhabricatorEditorSetting.php +++ b/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(); } } diff --git a/src/infrastructure/editor/PhabricatorEditorURIEngine.php b/src/infrastructure/editor/PhabricatorEditorURIEngine.php new file mode 100644 index 0000000000..43bfc23a5b --- /dev/null +++ b/src/infrastructure/editor/PhabricatorEditorURIEngine.php @@ -0,0 +1,335 @@ +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; + } + +} diff --git a/src/infrastructure/editor/PhabricatorEditorURIParserException.php b/src/infrastructure/editor/PhabricatorEditorURIParserException.php new file mode 100644 index 0000000000..f0421dad16 --- /dev/null +++ b/src/infrastructure/editor/PhabricatorEditorURIParserException.php @@ -0,0 +1,4 @@ + 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)); + } + } + +}