From 90e75d4e50be5900f7f797d966878a2a63daa6d3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 1 Jul 2014 11:04:05 -0700 Subject: [PATCH] Add flat text assertions to Phabricator remarkup rules Summary: Remarkup rules can not safely use arbitrary text in tag attributes, because it may include tokens which are later replaced. Precedence rules should prevent this in general. Use flat text assertions and adjust precedence rules in cases where they may not prevent tokens from appearing in attributes. Auditors: btrahan --- resources/celerity/map.php | 44 +++++++++---------- .../markup/DivinerRemarkupRuleSymbol.php | 8 +++- .../remarkup/DoorkeeperRemarkupRule.php | 4 +- .../PhabricatorRemarkupRuleEmbedFile.php | 8 ++-- .../remarkup/PhabricatorRemarkupRuleIcon.php | 6 +-- .../PhabricatorRemarkupRuleImageMacro.php | 2 +- .../remarkup/PhabricatorRemarkupRuleMeme.php | 8 +++- .../remarkup/PhabricatorPasteRemarkupRule.php | 3 +- .../remarkup/PhrictionRemarkupRule.php | 2 +- .../rule/PhabricatorRemarkupRuleObject.php | 12 +++-- .../rule/PhabricatorRemarkupRuleYoutube.php | 9 ++-- 11 files changed, 61 insertions(+), 45 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 3645ce0536..9fd62867bb 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -8,10 +8,10 @@ return array( 'names' => array( 'core.pkg.css' => 'c2c68e64', - 'core.pkg.js' => '834b4eda', + 'core.pkg.js' => 'f8ec7ddc', 'darkconsole.pkg.js' => 'df001cab', 'differential.pkg.css' => '4a93db37', - 'differential.pkg.js' => 'd1443567', + 'differential.pkg.js' => '7528cfc9', 'diffusion.pkg.css' => '471bc9eb', 'diffusion.pkg.js' => 'bfc0737b', 'maniphest.pkg.css' => 'f5d89daf', @@ -362,7 +362,7 @@ return array( 'rsrc/js/application/differential/ChangesetViewManager.js' => 'd2907473', 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => 'f2441746', 'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => 'e10f8e18', - 'rsrc/js/application/differential/behavior-comment-jump.js' => '2bc7a5e0', + 'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d', 'rsrc/js/application/differential/behavior-comment-preview.js' => '127f2018', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', 'rsrc/js/application/differential/behavior-dropdown-menus.js' => '710f209e', @@ -473,7 +473,7 @@ return array( 'rsrc/js/core/behavior-object-selector.js' => '39841ead', 'rsrc/js/core/behavior-oncopy.js' => '2926fff2', 'rsrc/js/core/behavior-phabricator-nav.js' => '14d7a8b8', - 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => 'eff6a142', + 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => 'e32d14ab', 'rsrc/js/core/behavior-refresh-csrf.js' => '7814b593', 'rsrc/js/core/behavior-remarkup-preview.js' => 'f7379f45', 'rsrc/js/core/behavior-reorder-applications.js' => '76b9fc3e', @@ -567,7 +567,7 @@ return array( 'javelin-behavior-dashboard-tab-panel' => 'd4eecc63', 'javelin-behavior-device' => '03d6ed07', 'javelin-behavior-differential-add-reviewers-and-ccs' => 'e10f8e18', - 'javelin-behavior-differential-comment-jump' => '2bc7a5e0', + 'javelin-behavior-differential-comment-jump' => '4fdb476d', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-dropdown-menus' => '710f209e', 'javelin-behavior-differential-edit-inline-comments' => '00861799', @@ -621,7 +621,7 @@ return array( 'javelin-behavior-phabricator-notification-example' => '7a9677fc', 'javelin-behavior-phabricator-object-selector' => '39841ead', 'javelin-behavior-phabricator-oncopy' => '2926fff2', - 'javelin-behavior-phabricator-remarkup-assist' => 'eff6a142', + 'javelin-behavior-phabricator-remarkup-assist' => 'e32d14ab', 'javelin-behavior-phabricator-reveal-content' => '60821bc7', 'javelin-behavior-phabricator-search-typeahead' => '5a376f34', 'javelin-behavior-phabricator-show-all-transactions' => '7c273581', @@ -1047,12 +1047,6 @@ return array( 3 => 'javelin-workflow', 4 => 'javelin-json', ), - '2bc7a5e0' => - array( - 0 => 'javelin-behavior', - 1 => 'javelin-stratcom', - 2 => 'javelin-dom', - ), '2cad29d1' => array( 0 => 'javelin-install', @@ -1225,6 +1219,12 @@ return array( 3 => 'javelin-stratcom', 4 => 'javelin-request', ), + '4fdb476d' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-stratcom', + 2 => 'javelin-dom', + ), '519705ea' => array( 0 => 'javelin-install', @@ -1952,6 +1952,16 @@ return array( 1 => 'javelin-stratcom', 2 => 'javelin-dom', ), + 'e32d14ab' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-stratcom', + 2 => 'javelin-dom', + 3 => 'phabricator-phtize', + 4 => 'phabricator-textareautils', + 5 => 'javelin-workflow', + 6 => 'javelin-vector', + ), 'e379b58e' => array( 0 => 'javelin-behavior', @@ -1995,16 +2005,6 @@ return array( 0 => 'javelin-install', 1 => 'javelin-util', ), - 'eff6a142' => - array( - 0 => 'javelin-behavior', - 1 => 'javelin-stratcom', - 2 => 'javelin-dom', - 3 => 'phabricator-phtize', - 4 => 'phabricator-textareautils', - 5 => 'javelin-workflow', - 6 => 'javelin-vector', - ), 'f0a41b9f' => array( 0 => 'javelin-behavior', diff --git a/src/applications/diviner/markup/DivinerRemarkupRuleSymbol.php b/src/applications/diviner/markup/DivinerRemarkupRuleSymbol.php index 0e9aab53eb..b851555f07 100644 --- a/src/applications/diviner/markup/DivinerRemarkupRuleSymbol.php +++ b/src/applications/diviner/markup/DivinerRemarkupRuleSymbol.php @@ -4,6 +4,10 @@ final class DivinerRemarkupRuleSymbol extends PhutilRemarkupRule { const KEY_RULE_ATOM_REF = 'rule.diviner.atomref'; + public function getPriority() { + return 40.0; + } + public function apply($text) { // Grammar here is: // @@ -132,7 +136,7 @@ final class DivinerRemarkupRuleSymbol extends PhutilRemarkupRule { $link = $title; } } else if ($href) { - $link = phutil_tag( + $link = $this->newTag( 'a', array( 'class' => 'atom-ref', @@ -140,7 +144,7 @@ final class DivinerRemarkupRuleSymbol extends PhutilRemarkupRule { ), $title); } else { - $link = phutil_tag( + $link = $this->newTag( 'span', array( 'class' => 'atom-ref-invalid', diff --git a/src/applications/doorkeeper/remarkup/DoorkeeperRemarkupRule.php b/src/applications/doorkeeper/remarkup/DoorkeeperRemarkupRule.php index a8988e910f..0aaad3e762 100644 --- a/src/applications/doorkeeper/remarkup/DoorkeeperRemarkupRule.php +++ b/src/applications/doorkeeper/remarkup/DoorkeeperRemarkupRule.php @@ -48,8 +48,8 @@ abstract class DoorkeeperRemarkupRule } else { $view = id(new PHUITagView()) ->setID($tag_id) - ->setName($spec['href']) - ->setHref($spec['href']) + ->setName($this->assertFlatText($spec['href'])) + ->setHref($this->assertFlatText($spec['href'])) ->setType(PHUITagView::TYPE_OBJECT) ->setExternal(true); } diff --git a/src/applications/files/remarkup/PhabricatorRemarkupRuleEmbedFile.php b/src/applications/files/remarkup/PhabricatorRemarkupRuleEmbedFile.php index b720921254..40f55afd21 100644 --- a/src/applications/files/remarkup/PhabricatorRemarkupRuleEmbedFile.php +++ b/src/applications/files/remarkup/PhabricatorRemarkupRuleEmbedFile.php @@ -180,7 +180,7 @@ final class PhabricatorRemarkupRuleEmbedFile $autoplay = null; } - return phutil_tag( + return $this->newTag( 'audio', array( 'controls' => 'controls', @@ -188,7 +188,7 @@ final class PhabricatorRemarkupRuleEmbedFile 'autoplay' => $autoplay, 'loop' => idx($options, 'loop') ? 'loop' : null, ), - phutil_tag( + $this->newTag( 'source', array( 'src' => $file->getBestURI(), @@ -203,10 +203,10 @@ final class PhabricatorRemarkupRuleEmbedFile return id(new PhabricatorFileLinkView()) ->setFilePHID($file->getPHID()) - ->setFileName($options['name']) + ->setFileName($this->assertFlatText($options['name'])) ->setFileDownloadURI($file->getDownloadURI()) ->setFileViewURI($file->getBestURI()) - ->setFileViewable($options['viewable']); + ->setFileViewable((bool)$options['viewable']); } private function parseDimension($string) { diff --git a/src/applications/macro/remarkup/PhabricatorRemarkupRuleIcon.php b/src/applications/macro/remarkup/PhabricatorRemarkupRuleIcon.php index 861791581b..0e59f94e64 100644 --- a/src/applications/macro/remarkup/PhabricatorRemarkupRuleIcon.php +++ b/src/applications/macro/remarkup/PhabricatorRemarkupRuleIcon.php @@ -3,9 +3,9 @@ final class PhabricatorRemarkupRuleIcon extends PhutilRemarkupRule { - private $macros; - - const KEY_RULE_MACRO = 'rule.macro'; + public function getPriority() { + return 50.0; + } public function apply($text) { return preg_replace_callback( diff --git a/src/applications/macro/remarkup/PhabricatorRemarkupRuleImageMacro.php b/src/applications/macro/remarkup/PhabricatorRemarkupRuleImageMacro.php index 8f2ac2c2ab..1c9ba594d6 100644 --- a/src/applications/macro/remarkup/PhabricatorRemarkupRuleImageMacro.php +++ b/src/applications/macro/remarkup/PhabricatorRemarkupRuleImageMacro.php @@ -147,7 +147,7 @@ final class PhabricatorRemarkupRuleImageMacro )); } - $result = phutil_tag( + $result = $this->newTag( 'img', array( 'id' => $id, diff --git a/src/applications/macro/remarkup/PhabricatorRemarkupRuleMeme.php b/src/applications/macro/remarkup/PhabricatorRemarkupRuleMeme.php index 88c754ff37..242f2a8db1 100644 --- a/src/applications/macro/remarkup/PhabricatorRemarkupRuleMeme.php +++ b/src/applications/macro/remarkup/PhabricatorRemarkupRuleMeme.php @@ -8,6 +8,10 @@ final class PhabricatorRemarkupRuleMeme private $images; + public function getPriority() { + return 50.0; + } + public function apply($text) { return preg_replace_callback( '@{meme,((?:[^}\\\\]+|\\\\.)+)}$@m', @@ -42,10 +46,10 @@ final class PhabricatorRemarkupRuleMeme $options['above'], $options['below']); - $img = phutil_tag( + $img = $this->newTag( 'img', array( - 'src' => (string)$uri, + 'src' => $uri, 'alt' => $alt_text, )); } diff --git a/src/applications/paste/remarkup/PhabricatorPasteRemarkupRule.php b/src/applications/paste/remarkup/PhabricatorPasteRemarkupRule.php index 1115bac6c4..8185c39fb8 100644 --- a/src/applications/paste/remarkup/PhabricatorPasteRemarkupRule.php +++ b/src/applications/paste/remarkup/PhabricatorPasteRemarkupRule.php @@ -54,7 +54,6 @@ final class PhabricatorPasteRemarkupRule } - return $embed_paste->render(); - + return $embed_paste; } } diff --git a/src/applications/phriction/remarkup/PhrictionRemarkupRule.php b/src/applications/phriction/remarkup/PhrictionRemarkupRule.php index 7a0acc88ff..c6ae9f5b14 100644 --- a/src/applications/phriction/remarkup/PhrictionRemarkupRule.php +++ b/src/applications/phriction/remarkup/PhrictionRemarkupRule.php @@ -38,7 +38,7 @@ final class PhrictionRemarkupRule } else if ($this->getEngine()->isTextMode()) { return PhabricatorEnv::getProductionURI($href); } else { - $text = phutil_tag( + $text = $this->newTag( 'a', array( 'href' => $href, diff --git a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObject.php b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObject.php index f49a1d583d..ee5fd3a2b4 100644 --- a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObject.php +++ b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleObject.php @@ -12,6 +12,10 @@ abstract class PhabricatorRemarkupRuleObject abstract protected function getObjectNamePrefix(); abstract protected function loadObjects(array $ids); + public function getPriority() { + return 50.0; + } + protected function getObjectNamePrefixBeginsWithWordCharacter() { $prefix = $this->getObjectNamePrefix(); return preg_match('/^\w/', $prefix); @@ -116,11 +120,12 @@ abstract class PhabricatorRemarkupRuleObject } // NOTE: The "(?assertFlatText($spec['options']); $view = $this->renderObjectEmbed($object, $handle, $spec['options']); break; } diff --git a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleYoutube.php b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleYoutube.php index ba3b0c92dc..2627468896 100644 --- a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleYoutube.php +++ b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleYoutube.php @@ -30,9 +30,12 @@ final class PhabricatorRemarkupRuleYoutube } $youtube_src = 'https://www.youtube.com/embed/'.$v; - $iframe = phutil_tag_div( - 'embedded-youtube-video', - phutil_tag( + $iframe = $this->newTag( + 'div', + array( + 'class' => 'embedded-youtube-video', + ), + $this->newTag( 'iframe', array( 'width' => '650', -- 2.54.0