Page MenuHomePhabricator
Files F15423410

D8548.id20282.diff
No OneTemporary
Actions

D8548.id20282.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
@@ -68,13 +68,41 @@
public function testTagJavascriptProtocolRejection() {
$hrefs = array(
'javascript:alert(1)' => true,
- 'JAVASCRIPT:alert(1)' => true,
- ' javascript:alert(1)' => true,
+ 'JAVASCRIPT:alert(2)' => true,
+ ' javascript:alert(3)' => true,
'/' => false,
'/path/to/stuff/' => false,
'' => false,
'http://example.com/' => false,
'#' => false,
+ 'javascript://anything' => true,
+
+ // Chrome 33 and IE11, at a minimum, treat this as Javascript.
+ "javascript\n:alert(4)" => true,
+
+ // Opera currently accepts a variety of unicode spaces. This test case
+ // has a smattering of them.
+ "\xE2\x80\x89javascript:" => true,
+ "javascript\xE2\x80\x89:" => true,
+ "\xE2\x80\x84javascript:" => true,
+ "javascript\xE2\x80\x84:" => true,
+
+ // Because we're aggressive, all of unicode should trigger detection
+ // by default.
+ "\xE2\x98\x83javascript:" => true,
+ "javascript\xE2\x98\x83:" => true,
+ "\xE2\x98\x83javascript\xE2\x98\x83:" => true,
+
+ // We're aggressive about this, so we'll intentionally raise false
+ // positives in these cases.
+ 'javascript~:alert(5)' => true,
+ '!!!javascript!!!!:alert(6)' => true,
+
+ // However, we should raise true negatives in these slightly more
+ // reasonable cases.
+ 'javascript/:docs.html' => false,
+ 'javascripts:x.png' => false,
+ 'COOLjavascript:page' => false,
);
foreach (array(true, false) as $use_uri) {
diff --git a/src/markup/render.php b/src/markup/render.php
--- a/src/markup/render.php
+++ b/src/markup/render.php
@@ -10,17 +10,28 @@
$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. This function
- // is deep in the core and performance sensitive, so skip the relatively
- // expensive preg_match() call if the initial character is '/' (this is the
- // case with essentially every URI Phabricator renders).
- if (isset($href[0]) &&
- ($href[0] != '/') &&
- preg_match('/^\s*javascript:/i', $href)) {
- throw new Exception(
- "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.");
+ // should ever use them, and they are a potent attack vector.
+
+ // 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.
+
+ if (isset($href[0]) && ($href[0] != '/')) {
+
+ // 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
Sun, Mar 23, 3:21 PM (1 d, 21 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7718664
Default Alt Text
D8548.id20282.diff (3 KB)

Event Timeline

Phabricator · Built by Phacility