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 @@ -5771,6 +5771,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', @@ -12668,6 +12669,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,14 @@ return $name; } - $same_window = $engine->getConfig('uri.same-window', false); + // 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 + $is_internal = self::isURIInternal($link); + $same_window = $engine->getConfig('uri.same-window', $is_internal); + if ($same_window) { $target = null; } else { @@ -88,11 +95,18 @@ $target = null; } + // Allow developers to style esternal links differently + $classes = array('remarkup-link'); + if (!$is_internal) { + $classes[] = 'remarkup-link-ext'; + } + $classes = implode(' ', $classes); + return phutil_tag( 'a', array( 'href' => $link, - 'class' => 'remarkup-link', + 'class' => $classes, 'target' => $target, 'rel' => 'noreferrer', ), 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,18 +116,27 @@ $engine = $this->getEngine(); - $same_window = $engine->getConfig('uri.same-window', false); + $is_internal = self::isURIAbsoluteInternal($link); + $same_window = $engine->getConfig('uri.same-window', $is_internal); + if ($same_window) { $target = null; } else { $target = '_blank'; } + // Allow web designers to style external links differently + $classes = array('remarkup-link'); + if (!$is_internal) { + $classes[] = 'remarkup-link-ext'; + } + $classes = implode(' ', $classes); + return phutil_tag( 'a', array( 'href' => $link, - 'class' => 'remarkup-link', + 'class' => $classes, 'target' => $target, 'rel' => 'noreferrer', ), 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,73 @@ 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) { + // is this starting with '/' or './' or '../'? + return + strncmp($uri, '/', 1) === 0 || + strncmp($uri, './', 2) === 0 || + strncmp($uri, '../', 3) === 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; + } + + // Check if the URI starts with the base URI. + return strncmp($uri, $base_uri, strlen($base_uri)) === 0; + } + } 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); + } + } +}