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 @@ -5770,6 +5770,7 @@ 'PhutilRemarkupQuotesBlockRule' => 'infrastructure/markup/blockrule/PhutilRemarkupQuotesBlockRule.php', 'PhutilRemarkupReplyBlockRule' => 'infrastructure/markup/blockrule/PhutilRemarkupReplyBlockRule.php', 'PhutilRemarkupRule' => 'infrastructure/markup/markuprule/PhutilRemarkupRule.php', + 'PhutilRemarkupRuleTestCase' => 'infrastructure/markup/markuprule/__tests__/PhutilRemarkupRuleTestCase.php', 'PhutilRemarkupSimpleTableBlockRule' => 'infrastructure/markup/blockrule/PhutilRemarkupSimpleTableBlockRule.php', 'PhutilRemarkupTableBlockRule' => 'infrastructure/markup/blockrule/PhutilRemarkupTableBlockRule.php', 'PhutilRemarkupTestInterpreterRule' => 'infrastructure/markup/blockrule/PhutilRemarkupTestInterpreterRule.php', @@ -12666,6 +12667,7 @@ 'PhutilRemarkupQuotesBlockRule' => 'PhutilRemarkupQuotedBlockRule', 'PhutilRemarkupReplyBlockRule' => 'PhutilRemarkupQuotedBlockRule', 'PhutilRemarkupRule' => 'Phobject', + 'PhutilRemarkupRuleTestCase' => 'PhabricatorTestCase', 'PhutilRemarkupSimpleTableBlockRule' => 'PhutilRemarkupBlockRule', 'PhutilRemarkupTableBlockRule' => 'PhutilRemarkupBlockRule', 'PhutilRemarkupTestInterpreterRule' => 'PhutilRemarkupBlockInterpreter', diff --git a/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php b/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php --- a/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php +++ b/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php @@ -45,11 +45,11 @@ $engine = $this->getEngine(); $is_anchor = false; - if (strncmp($link, '/', 1) == 0) { + if (self::isURIRelativePath($link)) { $base = phutil_string_cast($engine->getConfig('uri.base')); $base = rtrim($base, '/'); $link = $base.$link; - } else if (strncmp($link, '#', 1) == 0) { + } else if (self::isURIAnchor($link)) { $here = $engine->getConfig('uri.here'); $link = $here.$link; @@ -76,7 +76,16 @@ return $name; } - $same_window = $engine->getConfig('uri.same-window', false); + $same_window = $engine->getConfig('uri.same-window', null); + if ($same_window === null) { + // Instead of assuming every link as external, + // assume that only external resources should be opened + // externally. This still probably creates lot of + // external links than needed, but less than before. + // https://we.phorge.it/T15161 + $same_window = self::isURIInternal($link); + } + if ($same_window) { $target = null; } else { diff --git a/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php b/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php --- a/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php +++ b/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php @@ -116,7 +116,16 @@ $engine = $this->getEngine(); - $same_window = $engine->getConfig('uri.same-window', false); + $same_window = $engine->getConfig('uri.same-window', null); + if ($same_window === null) { + // Instead of assuming every link as external, + // assume that only external resources should be opened + // externally. This still probably creates lot of + // external links than needed, but less than before. + // https://we.phorge.it/T15161 + $same_window = self::isURIAbsoluteInternal($link); + } + if ($same_window) { $target = null; } else { diff --git a/src/infrastructure/markup/markuprule/PhutilRemarkupRule.php b/src/infrastructure/markup/markuprule/PhutilRemarkupRule.php --- a/src/infrastructure/markup/markuprule/PhutilRemarkupRule.php +++ b/src/infrastructure/markup/markuprule/PhutilRemarkupRule.php @@ -106,4 +106,86 @@ return (strpos($text, PhutilRemarkupBlockStorage::MAGIC_BYTE) === false); } + /** + * Check if a generic URI points to an internal destination + * + * The URI can be relative or absolute + * + * WARNING: This method is public just to make unit tests, so, + * don't adopt this method in production externally since it can + * change anytime. + * If you need to check this from external places, please + * propose to move the business logic somewhere else, but please + * also indicating where. + * https://we.phorge.it/T15182 + * + * @param string $uri Example '/bar/' or 'http://example.com/bar/' + * @return bool + */ + public static function isURIInternal($uri) { + return self::isURIAnchor($uri) + || self::isURIRelativePath($uri) + || self::isURIAbsoluteInternal($uri); + } + + /** + * Check whenever an URI is an anchor + * + * @param string $uri + * @return bool + */ + protected static function isURIAnchor($uri) { + return strncmp($uri, '#', 1) === 0; + } + + /** + * Check whenever an URI is a relative one + * + * @param string $uri Example '/path' etc. + * @return bool + */ + protected static function isURIRelativePath($uri) { + // NOTE: This was the original check, but at + // the moment '/' is assumed as relative but + // './' is assumed as not relative, and this + // may be confusing and may deserve a future update + return strncmp($uri, '/', 1) === 0; + } + + /** + * Check if an absolute URI points to Phorge/Phabricator itself + * + * @param string $uri Example 'http://example.com/bar/' + * @return bool + */ + protected static function isURIAbsoluteInternal($uri) { + + // Get the Base URI - full URI with protocol and ending with '/' + // It's difficult to imagine a case where this config is not defined, + // but better to be nice here + $base_uri = + PhabricatorEnv::getEnvConfigIfExists('phabricator.base-uri', null); + + // Assume a safe default + if (!$base_uri) { + return false; + } + + return self::isURIStartingWith($uri, $base_uri); + } + + /** + * Check whenever an URI starts with a specific prefix + * + * @param $uri string Example 'http://example.com/bar/' + * @param $prefix string Example 'http://example.com/' + * @return bool True if the first is contained in the second + */ + private static function isURIStartingWith($uri, $prefix) { + // I know that PHP 8 has 'str_starts_with()' + // but that is not the minimum version + // https://stackoverflow.com/a/2790919 + return substr($uri, 0, strlen($prefix)) === $prefix; + } + } diff --git a/src/infrastructure/markup/markuprule/__tests__/PhutilRemarkupRuleTestCase.php b/src/infrastructure/markup/markuprule/__tests__/PhutilRemarkupRuleTestCase.php new file mode 100644 --- /dev/null +++ b/src/infrastructure/markup/markuprule/__tests__/PhutilRemarkupRuleTestCase.php @@ -0,0 +1,34 @@ +assertEqual($tested_value, $test_expected, $test_name); + } + } +}