Page MenuHomePhabricator
Files F13954975

D8711.id20657.diff
No OneTemporary
Actions

D8711.id20657.diff
View Options

diff --git a/src/markup/__tests__/PhutilMarkupTestCase.php b/src/markup/__tests__/PhutilMarkupTestCase.php
--- a/src/markup/__tests__/PhutilMarkupTestCase.php
+++ b/src/markup/__tests__/PhutilMarkupTestCase.php
@@ -65,6 +65,56 @@
(string)phutil_tag('br', array('y' => null)));
}
+ public function testDefaultRelNoreferrer() {
+ $map = array(
+ // These should not have rel="nofollow" inserted implicitly.
+ '/' => false,
+ '/path/to/local.html' => false,
+ '#example' => false,
+ '' => false,
+
+ // These should get the implicit insertion.
+ 'http://www.example.org/' => true,
+ '///evil.com/' => true,
+ ' http://www.example.org/' => true,
+ 'ftp://filez.com' => true,
+ 'mailto:santa@northpole.com' => true,
+ );
+
+ foreach ($map as $input => $expect) {
+ $tag = phutil_tag(
+ 'a',
+ array(
+ 'href' => $input,
+ ),
+ 'link');
+ $tag = (string)$tag;
+ $this->assertEqual($expect, (bool)preg_match('/noreferrer/', $tag));
+ }
+
+ // With an explicit `rel` present, we should not override it.
+ $tag = phutil_tag(
+ 'a',
+ array(
+ 'href' => 'http://www.example.org/',
+ 'rel' => 'nofollow',
+ ),
+ 'link');
+
+ $this->assertFalse((bool)preg_match('/noreferrer/', (string)$tag));
+
+ // For tags other than `a`, we should not insert `rel`.
+ $tag = phutil_tag(
+ 'link',
+ array(
+ 'href' => 'http://www.example.org/',
+ ),
+ 'link');
+
+ $this->assertFalse((bool)preg_match('/noreferrer/', (string)$tag));
+ }
+
+
public function testTagJavascriptProtocolRejection() {
$hrefs = array(
'javascript:alert(1)' => true,
diff --git a/src/markup/render.php b/src/markup/render.php
--- a/src/markup/render.php
+++ b/src/markup/render.php
@@ -1,36 +1,79 @@
<?php
/**
- * @group markup
+ * Render an HTML tag in a way that treats user content as unsafe by default.
+ *
+ * Tag rendering has some special logic which implements security features:
+ *
+ * - When rendering `<a>` tags, if the `rel` attribute is not specified, it
+ * is interpreted as `rel="noreferrer"`.
+ * - When rendering `<a>` tags, the `href` attribute may not begin with
+ * `javascript:`.
+ *
+ * These special cases can not be disabled.
+ *
+ * IMPORTANT: The `$tag` attribute and the keys of the `$attributes` array are
+ * trusted blindly, and not escaped. You should not pass user data in these
+ * parameters.
+ *
+ * @param string The name of the tag, like `a` or `div`.
+ * @param map<string, string> A map of tag attributes.
+ * @param wild Content to put in the tag.
+ * @return PhutilSafeHTML Tag object.
*/
function phutil_tag($tag, array $attributes = array(), $content = null) {
+
+ // If the `href` attribute is present:
+ // - make sure it is not a "javascript:" URI. We never permit these.
+ // - if the tag is an `<a>` and the link is to some foreign resource,
+ // add `rel="nofollow"` by default.
if (!empty($attributes['href'])) {
// This might be a URI object, so cast it to a string.
$href = (string)$attributes['href'];
- // Block 'javascript:' hrefs at the tag level: no well-designed application
- // should ever use them, and they are a potent attack vector.
+ if (isset($href[0])) {
+ $is_anchor_href = ($href[0] == '#');
- // This function is deep in the core and performance sensitive, so we're
- // doing a cheap version of this test first to avoid calling preg_match()
- // on URIs which begin with '/'. This covers essentially all URIs in
- // Phabricator.
+ // Is this a link to a resource on the same domain? The second part of
+ // this excludes "///evil.com/" protocol-relative hrefs.
+ $is_domain_href = ($href[0] == '/') &&
+ (!isset($href[1]) || $href[1] != '/');
- if (isset($href[0]) && ($href[0] != '/')) {
+ // If the `rel` attribute is not specified, fill in `rel="noreferrer"`.
+ // Effectively, this serves to make the default behavior for offsite
+ // links "do not send a referrer", which is broadly desirable. Specifying
+ // some non-null `rel` will skip this.
+ if (!isset($attributes['rel'])) {
+ if (!$is_anchor_href && !$is_domain_href) {
+ if ($tag == 'a') {
+ $attributes['rel'] = 'noreferrer';
+ }
+ }
+ }
- // Chrome 33 and IE 11 both interpret "javascript\n:" as a Javascript URI,
- // and all browsers interpret " javascript:" as a Javascript URI, so be
- // aggressive about looking for "javascript:" in the initial section of
- // the string.
+ // Block 'javascript:' hrefs at the tag level: no well-designed
+ // application should ever use them, and they are a potent attack vector.
- $normalized_href = preg_replace('([^a-z0-9/:]+)i', '', $href);
- if (preg_match('/^javascript:/i', $normalized_href)) {
- throw new Exception(
- pht(
- "Attempting to render a tag with an 'href' attribute that begins ".
- "with 'javascript:'. This is either a serious security concern ".
- "or a serious architecture concern. Seek urgent remedy."));
+ // This function is deep in the core and performance sensitive, so we're
+ // doing a cheap version of this test first to avoid calling preg_match()
+ // on URIs which begin with '/' or `#`. These cover essentially all URIs
+ // in Phabricator.
+ if (!$is_anchor_href && !$is_domain_href) {
+ // Chrome 33 and IE 11 both interpret "javascript\n:" as a Javascript
+ // URI, and all browsers interpret " javascript:" as a Javascript URI,
+ // so be aggressive about looking for "javascript:" in the initial
+ // section of the string.
+
+ $normalized_href = preg_replace('([^a-z0-9/:]+)i', '', $href);
+ if (preg_match('/^javascript:/i', $normalized_href)) {
+ throw new Exception(
+ pht(
+ "Attempting to render a tag with an 'href' attribute that ".
+ "begins with 'javascript:'. This is either a serious security ".
+ "concern or a serious architecture concern. Seek urgent ".
+ "remedy."));
+ }
}
}
}

File Metadata

Mime Type
text/plain
Expires
Oct 14 2024, 11:43 PM (4 w, 3 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6710444
Default Alt Text
D8711.id20657.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility