Page MenuHomePhabricator
Files F15315789

D21143.id50340.diff
No OneTemporary
Actions

D21143.id50340.diff
View Options

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',
@@ -9734,6 +9736,9 @@
'PhabricatorEditorExtensionModule' => 'PhabricatorConfigModule',
'PhabricatorEditorMailEngineExtension' => 'PhabricatorMailEngineExtension',
'PhabricatorEditorSetting' => 'PhabricatorStringSetting',
+ 'PhabricatorEditorURIEngine' => 'Phobject',
+ 'PhabricatorEditorURIEngineTestCase' => 'PhabricatorTestCase',
+ 'PhabricatorEditorURIParserException' => 'Exception',
'PhabricatorElasticFulltextStorageEngine' => 'PhabricatorFulltextStorageEngine',
'PhabricatorElasticsearchHost' => 'PhabricatorSearchHost',
'PhabricatorElasticsearchSetupCheck' => 'PhabricatorSetupCheck',
@@ -10053,7 +10058,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<application>\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 @@
-<?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;
- }
-
-
-}
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 @@
+<?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;
+ }
+
+}
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 @@
+<?php
+
+final class PhabricatorEditorURIParserException
+ extends Exception {}
diff --git a/src/infrastructure/editor/__tests__/PhabricatorEditorURIEngineTestCase.php b/src/infrastructure/editor/__tests__/PhabricatorEditorURIEngineTestCase.php
new file mode 100644
--- /dev/null
+++ b/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));
+ }
+ }
+
+}

File Metadata

Mime Type
text/plain
Expires
Fri, Mar 7, 6:10 PM (2 w, 2 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7341800
Default Alt Text
D21143.id50340.diff (22 KB)

Event Timeline

Phabricator · Built by Phacility