diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3275,6 +3275,9 @@ '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 @@ '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 @@ 'PhabricatorEditorExtensionModule' => 'PhabricatorConfigModule', 'PhabricatorEditorMailEngineExtension' => 'PhabricatorMailEngineExtension', 'PhabricatorEditorSetting' => 'PhabricatorStringSetting', + 'PhabricatorEditorURIEngine' => 'Phobject', + 'PhabricatorEditorURIEngineTestCase' => 'PhabricatorTestCase', + 'PhabricatorEditorURIParserException' => 'Exception', 'PhabricatorElasticFulltextStorageEngine' => 'PhabricatorFulltextStorageEngine', 'PhabricatorElasticsearchHost' => 'PhabricatorSearchHost', 'PhabricatorElasticsearchSetupCheck' => 'PhabricatorSetupCheck', @@ -10054,7 +10059,6 @@ '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 --- a/src/applications/console/plugin/DarkConsoleErrorLogPlugin.php +++ b/src/applications/console/plugin/DarkConsoleErrorLogPlugin.php @@ -65,20 +65,9 @@ $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 --- a/src/applications/differential/view/DifferentialChangesetDetailView.php +++ b/src/applications/differential/view/DifferentialChangesetDetailView.php @@ -252,17 +252,20 @@ } private function getEditorURI() { - $viewer = $this->getViewer(); - - if (!$viewer->isLoggedIn()) { + $repository = $this->getRepository(); + if (!$repository) { return null; } - $repository = $this->getRepository(); - if (!$repository) { + $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 @@ $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 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -478,8 +478,9 @@ $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 --- a/src/applications/help/application/PhabricatorHelpApplication.php +++ b/src/applications/help/application/PhabricatorHelpApplication.php @@ -18,7 +18,6 @@ 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 --- 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 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -471,41 +471,6 @@ 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 --- a/src/applications/settings/setting/PhabricatorEditorSetting.php +++ b/src/applications/settings/setting/PhabricatorEditorSetting.php @@ -43,26 +43,9 @@ 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 --- /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 --- /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)); + } + } + +}