From 6bb7a282b1b5eb06068d5182094c04ae5bdc51bf Mon Sep 17 00:00:00 2001 From: vrana Date: Tue, 5 Feb 2013 14:30:29 -0800 Subject: [PATCH] Convert AphrontFormControl to safe HTML Summary: Everything here now should properly handle plain strings and safe HTML. Test Plan: /settings/panel/display/ Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T2432 Differential Revision: https://secure.phabricator.com/D4826 --- .../PhabricatorConduitConsoleController.php | 6 +-- .../PhabricatorConfigEditController.php | 44 ++++++++--------- .../DifferentialDiffViewController.php | 24 +++++----- .../PhabricatorFileUploadController.php | 11 ++--- .../controller/HeraldRuleController.php | 13 ++--- .../ManiphestTaskEditController.php | 7 +-- .../PhabricatorPasteEditController.php | 7 ++- .../PhortuneMonthYearExpiryControl.php | 7 +-- ...ricatorSettingsPanelDisplayPreferences.php | 8 ++-- .../PhabricatorSlowvotePollController.php | 1 - .../control/AphrontFormCheckboxControl.php | 17 ++++--- src/view/form/control/AphrontFormControl.php | 48 ++++++++++--------- .../control/AphrontFormDividerControl.php | 2 +- .../form/control/AphrontFormImageControl.php | 11 ++--- .../control/AphrontFormRadioButtonControl.php | 25 +++++----- .../control/AphrontFormRecaptchaControl.php | 4 +- .../form/control/AphrontFormStaticControl.php | 2 +- .../form/control/AphrontFormSubmitControl.php | 2 +- .../AphrontFormToggleButtonsControl.php | 2 +- .../control/PhabricatorRemarkupControl.php | 2 + 20 files changed, 117 insertions(+), 126 deletions(-) diff --git a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php index a50e713c0c..61248cbbf1 100644 --- a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php +++ b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php @@ -53,18 +53,16 @@ final class PhabricatorConduitConsoleController } } - $error_description = array(); $error_types = $method_object->defineErrorTypes(); if ($error_types) { - $error_description[] = ''; - $error_description = implode("\n", $error_description); + $error_description = phutil_tag('ul', array(), $error_description); } else { $error_description = "This method does not raise any specific errors."; } diff --git a/src/applications/config/controller/PhabricatorConfigEditController.php b/src/applications/config/controller/PhabricatorConfigEditController.php index 5983bbc1ae..57ee007a08 100644 --- a/src/applications/config/controller/PhabricatorConfigEditController.php +++ b/src/applications/config/controller/PhabricatorConfigEditController.php @@ -137,7 +137,7 @@ final class PhabricatorConfigEditController array( 'class' => 'phabricator-remarkup', ), - phutil_safe_html($engine->getOutput($option, 'description'))); + $engine->getOutput($option, 'description')); $form ->setUser($user) @@ -419,23 +419,23 @@ final class PhabricatorConfigEditController } $table = array(); - $table[] = ''; - $table[] = ''.pht('Example').''; - $table[] = ''.pht('Value').''; - $table[] = ''; + $table[] = hsprintf( + '%s%s', + pht('Example'), + pht('Value')); foreach ($examples as $example) { list($value, $description) = $example; if ($value === null) { - $value = ''.pht('(empty)').''; + $value = phutil_tag('em', array(), pht('(empty)')); } else { - $value = nl2br(phutil_escape_html($value)); + $value = phutil_escape_html_newlines($value); } - $table[] = ''; - $table[] = ''.phutil_escape_html($description).''; - $table[] = ''.$value.''; - $table[] = ''; + $table[] = hsprintf( + '%s%s', + $description, + $value); } require_celerity_resource('config-options-css'); @@ -445,7 +445,7 @@ final class PhabricatorConfigEditController array( 'class' => 'config-option-table', ), - new PhutilSafeHTML(implode("\n", $table))); + $table); } private function renderDefaults(PhabricatorConfigOption $option) { @@ -467,10 +467,10 @@ final class PhabricatorConfigEditController $table = array(); - $table[] = ''; - $table[] = ''.pht('Source').''; - $table[] = ''.pht('Value').''; - $table[] = ''; + $table[] = hsprintf( + '%s%s', + pht('Source'), + pht('Value')); foreach ($stack as $key => $source) { $value = $source->getKeys( array( @@ -478,16 +478,16 @@ final class PhabricatorConfigEditController )); if (!array_key_exists($option->getKey(), $value)) { - $value = ''.pht('(empty)').''; + $value = phutil_tag('em', array(), pht('(empty)')); } else { $value = PhabricatorConfigJSON::prettyPrintJSON( $value[$option->getKey()]); } - $table[] = ''; - $table[] = ''.phutil_escape_html($source->getName()).''; - $table[] = ''.$value.''; - $table[] = ''; + $table[] = hsprintf( + '%s%s', + $source->getName(), + $value); } require_celerity_resource('config-options-css'); @@ -497,7 +497,7 @@ final class PhabricatorConfigEditController array( 'class' => 'config-option-table', ), - new PhutilSafeHTML(implode("\n", $table))); + $table); } } diff --git a/src/applications/differential/controller/DifferentialDiffViewController.php b/src/applications/differential/controller/DifferentialDiffViewController.php index d764f560e3..afe4847c8c 100644 --- a/src/applications/differential/controller/DifferentialDiffViewController.php +++ b/src/applications/differential/controller/DifferentialDiffViewController.php @@ -38,11 +38,11 @@ final class DifferentialDiffViewController extends DifferentialController { // TODO: implmenent optgroup support in AphrontFormSelectControl? $select = array(); - $select[] = ''; - $select[] = ''; - $select[] = ''; + $select[] = hsprintf('', pht('Create New Revision')); + $select[] = hsprintf( + '', + pht('Create a new Revision...')); + $select[] = hsprintf(''); $revision_data = new DifferentialRevisionListData( DifferentialRevisionListData::QUERY_OPEN_OWNED, @@ -50,7 +50,9 @@ final class DifferentialDiffViewController extends DifferentialController { $revisions = $revision_data->loadRevisions(); if ($revisions) { - $select[] = ''; + $select[] = hsprintf( + '', + pht('Update Existing Revision')); foreach ($revisions as $revision) { $select[] = phutil_tag( 'option', @@ -59,13 +61,13 @@ final class DifferentialDiffViewController extends DifferentialController { ), $revision->getTitle()); } - $select[] = ''; + $select[] = hsprintf(''); } - $select = - ''; + $select = phutil_tag( + 'select', + array('name' => 'revisionID'), + $select); $action_form = new AphrontFormView(); $action_form diff --git a/src/applications/files/controller/PhabricatorFileUploadController.php b/src/applications/files/controller/PhabricatorFileUploadController.php index 94a1130ca5..cab1f145f6 100644 --- a/src/applications/files/controller/PhabricatorFileUploadController.php +++ b/src/applications/files/controller/PhabricatorFileUploadController.php @@ -30,13 +30,12 @@ final class PhabricatorFileUploadController extends PhabricatorFileController { $instructions = id(new AphrontFormMarkupControl()) ->setControlID($support_id) ->setControlStyle('display: none') - ->setValue( - '

'. + ->setValue(hsprintf( + '

%s %s

', + pht('Drag and Drop:'), pht( - 'Drag and Drop: You can also upload files by '. - 'dragging and dropping them from your desktop onto this page or '. - 'the Phabricator home page.'). - '

'); + 'You can also upload files by dragging and dropping them from your '. + 'desktop onto this page or the Phabricator home page.'))); $form = id(new AphrontFormView()) ->setFlexible(true) diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index f61062520d..b5fee57e90 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -117,9 +117,10 @@ final class HeraldRuleController extends HeraldController { $form ->appendChild( id(new AphrontFormMarkupControl()) - ->setValue( - "This ${rule_type_name} rule triggers for " . - "${content_type_name}.")) + ->setValue(hsprintf( + "This %s rule triggers for %s.", + $rule_type_name, + $content_type_name))) ->appendChild( id(new AphrontFormInsetView()) ->setTitle('Conditions') @@ -154,9 +155,9 @@ final class HeraldRuleController extends HeraldController { 'mustcapture' => true, ), 'Create New Action')) - ->setDescription( - phutil_safe_html( - 'Take these actions '.$repetition_selector.' this rule matches:')) + ->setDescription(hsprintf( + 'Take these actions %s this rule matches:', + $repetition_selector)) ->setContent(javelin_tag( 'table', array( diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 5b258baf8f..23fcf30bee 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -455,11 +455,8 @@ final class ManiphestTaskEditController extends ManiphestController { )); if ($files) { - $file_display = array(); - foreach ($files as $file) { - $file_display[] = phutil_escape_html($file->getName()); - } - $file_display = implode('
', $file_display); + $file_display = mpull($files, 'getName'); + $file_display = array_interleave(phutil_tag('br'), $file_display); $form->appendChild( id(new AphrontFormMarkupControl()) diff --git a/src/applications/paste/controller/PhabricatorPasteEditController.php b/src/applications/paste/controller/PhabricatorPasteEditController.php index 63beb71670..29d0272209 100644 --- a/src/applications/paste/controller/PhabricatorPasteEditController.php +++ b/src/applications/paste/controller/PhabricatorPasteEditController.php @@ -164,10 +164,9 @@ final class PhabricatorPasteEditController extends PhabricatorPasteController { ->appendChild( id(new AphrontFormMarkupControl()) ->setLabel('Text') - ->setValue( - 'Paste text can not be edited. '. - $fork_link.' to create a new paste.' - )); + ->setValue(hsprintf( + 'Paste text can not be edited. %s to create a new paste.', + $fork_link))); } $submit = new AphrontFormSubmitControl(); diff --git a/src/applications/phortune/control/PhortuneMonthYearExpiryControl.php b/src/applications/phortune/control/PhortuneMonthYearExpiryControl.php index a4701aa11c..b9c48cd595 100644 --- a/src/applications/phortune/control/PhortuneMonthYearExpiryControl.php +++ b/src/applications/phortune/control/PhortuneMonthYearExpiryControl.php @@ -80,12 +80,7 @@ final class PhortuneMonthYearExpiryControl extends AphrontFormControl { 'sigil' => 'year-input', )); - return self::renderSingleView( - array( - $months_sel, - $years_sel - ) - ); + return hsprintf('%s%s', $months_sel, $years_sel); } } diff --git a/src/applications/settings/panel/PhabricatorSettingsPanelDisplayPreferences.php b/src/applications/settings/panel/PhabricatorSettingsPanelDisplayPreferences.php index 97b1a21c4b..b5b4d00d35 100644 --- a/src/applications/settings/panel/PhabricatorSettingsPanelDisplayPreferences.php +++ b/src/applications/settings/panel/PhabricatorSettingsPanelDisplayPreferences.php @@ -122,10 +122,10 @@ EXAMPLE; ->setValue($preferences->getPreference($pref_monospaced))) ->appendChild( id(new AphrontFormMarkupControl()) - ->setValue( - '
'.
-          phutil_escape_html($example_string).
-          '
')) + ->setValue(phutil_tag( + 'pre', + array('class' => 'PhabricatorMonospaced'), + $example_string))) ->appendChild( id(new AphrontFormRadioButtonControl()) ->setLabel('Monospaced Textareas') diff --git a/src/applications/slowvote/controller/PhabricatorSlowvotePollController.php b/src/applications/slowvote/controller/PhabricatorSlowvotePollController.php index 2bc75bc744..9440e476b4 100644 --- a/src/applications/slowvote/controller/PhabricatorSlowvotePollController.php +++ b/src/applications/slowvote/controller/PhabricatorSlowvotePollController.php @@ -112,7 +112,6 @@ final class PhabricatorSlowvotePollController $viewer_choices, $option); } - $option_markup = implode("\n", $option_markup); $comments_by_option = array(); switch ($poll->getMethod()) { diff --git a/src/view/form/control/AphrontFormCheckboxControl.php b/src/view/form/control/AphrontFormCheckboxControl.php index 00a85f52a9..4d452c448e 100644 --- a/src/view/form/control/AphrontFormCheckboxControl.php +++ b/src/view/form/control/AphrontFormCheckboxControl.php @@ -38,16 +38,15 @@ final class AphrontFormCheckboxControl extends AphrontFormControl { 'for' => $id, ), $box['label']); - $rows[] = - ''. - ''.$checkbox.''. - ''.$label.''. - ''; + $rows[] = hsprintf( + '%s%s', + $checkbox, + $label); } - return - ''. - implode("\n", $rows). - '
'; + return phutil_tag( + 'table', + array('class' => 'aphront-form-control-checkbox-layout'), + $rows); } } diff --git a/src/view/form/control/AphrontFormControl.php b/src/view/form/control/AphrontFormControl.php index 6bbdfe853e..0e21457e29 100644 --- a/src/view/form/control/AphrontFormControl.php +++ b/src/view/form/control/AphrontFormControl.php @@ -108,32 +108,32 @@ abstract class AphrontFormControl extends AphrontView { $custom_class = $this->getCustomControlClass(); if (strlen($this->getLabel())) { - $label = - ''; + $label = phutil_tag( + 'label', + array('class' => 'aphront-form-label'), + $this->getLabel()); } else { $label = null; $custom_class .= ' aphront-form-control-nolabel'; } - $input = - '
'. - $this->renderInput(). - '
'; + $input = phutil_tag( + 'div', + array('class' => 'aphront-form-input'), + $this->renderInput()); if (strlen($this->getError())) { $error = $this->getError(); if ($error === true) { - $error = - '
'. - 'Required'. - '
'; + $error = phutil_tag( + 'div', + array('class' => 'aphront-form-error aphront-form-required'), + 'Required'); } else { - $error = - '
'. - phutil_escape_html($error). - '
'; + $error = phutil_tag( + 'div', + array('class' => 'aphront-form-error'), + $error); } } else { $error = null; @@ -148,19 +148,21 @@ abstract class AphrontFormControl extends AphrontView { $caption = null; } - return phutil_render_tag( + return phutil_tag( 'div', array( 'class' => "aphront-form-control {$custom_class}", 'id' => $this->controlID, 'style' => $this->controlStyle, ), - $label. - $error. - $input. - $caption. + array( + $label, + $error, + $input, + $caption, - // TODO: Remove this once the redesign finishes up. - '
'); + // TODO: Remove this once the redesign finishes up. + phutil_tag('div', array('style' => 'clear: both;'), ''), + )); } } diff --git a/src/view/form/control/AphrontFormDividerControl.php b/src/view/form/control/AphrontFormDividerControl.php index 7b9090b9a5..b76d9e055b 100644 --- a/src/view/form/control/AphrontFormDividerControl.php +++ b/src/view/form/control/AphrontFormDividerControl.php @@ -7,7 +7,7 @@ final class AphrontFormDividerControl extends AphrontFormControl { } protected function renderInput() { - return '
'; + return phutil_tag('hr'); } } diff --git a/src/view/form/control/AphrontFormImageControl.php b/src/view/form/control/AphrontFormImageControl.php index ba2adb2c92..8e84d6fefc 100644 --- a/src/view/form/control/AphrontFormImageControl.php +++ b/src/view/form/control/AphrontFormImageControl.php @@ -9,14 +9,14 @@ final class AphrontFormImageControl extends AphrontFormControl { protected function renderInput() { $id = celerity_generate_unique_node_id(); - return + return hsprintf( + '%s
%s%s
', phutil_tag( 'input', array( 'type' => 'file', 'name' => $this->getName(), - )). - '
'. + )), phutil_tag( 'input', array( @@ -24,14 +24,13 @@ final class AphrontFormImageControl extends AphrontFormControl { 'name' => 'default_image', 'class' => 'default-image', 'id' => $id, - )). + )), phutil_tag( 'label', array( 'for' => $id, ), - 'Use Default Image instead'). - '
'; + 'Use Default Image instead')); } } diff --git a/src/view/form/control/AphrontFormRadioButtonControl.php b/src/view/form/control/AphrontFormRadioButtonControl.php index 6d6547c8b6..1fca18e9d5 100644 --- a/src/view/form/control/AphrontFormRadioButtonControl.php +++ b/src/view/form/control/AphrontFormRadioButtonControl.php @@ -43,22 +43,21 @@ final class AphrontFormRadioButtonControl extends AphrontFormControl { $button['label']); if (strlen($button['caption'])) { - $label .= - '
'. - phutil_escape_html($button['caption']). - '
'; + $label = hsprintf( + '%s
%s
', + $label, + $button['caption']); } - $rows[] = - ''. - ''.$radio.''. - ''.$label.''. - ''; + $rows[] = hsprintf( + '%s%s', + $radio, + $label); } - return - ''. - implode("\n", $rows). - '
'; + return phutil_tag( + 'table', + array('class' => 'aphront-form-control-radio-layout'), + $rows); } } diff --git a/src/view/form/control/AphrontFormRecaptchaControl.php b/src/view/form/control/AphrontFormRecaptchaControl.php index 25bc39792c..078eb1afc2 100644 --- a/src/view/form/control/AphrontFormRecaptchaControl.php +++ b/src/view/form/control/AphrontFormRecaptchaControl.php @@ -53,10 +53,10 @@ final class AphrontFormRecaptchaControl extends AphrontFormControl { $protocol = $uri->getProtocol(); $use_ssl = ($protocol == 'https'); - return recaptcha_get_html( + return phutil_safe_html(recaptcha_get_html( PhabricatorEnv::getEnvConfig('recaptcha.public-key'), $error = null, - $use_ssl); + $use_ssl)); } } diff --git a/src/view/form/control/AphrontFormStaticControl.php b/src/view/form/control/AphrontFormStaticControl.php index d72fa37586..47060ea292 100644 --- a/src/view/form/control/AphrontFormStaticControl.php +++ b/src/view/form/control/AphrontFormStaticControl.php @@ -7,7 +7,7 @@ final class AphrontFormStaticControl extends AphrontFormControl { } protected function renderInput() { - return phutil_escape_html($this->getValue()); + return $this->getValue(); } } diff --git a/src/view/form/control/AphrontFormSubmitControl.php b/src/view/form/control/AphrontFormSubmitControl.php index c2ccb915e5..a6f505f905 100644 --- a/src/view/form/control/AphrontFormSubmitControl.php +++ b/src/view/form/control/AphrontFormSubmitControl.php @@ -30,7 +30,7 @@ final class AphrontFormSubmitControl extends AphrontFormControl { ), $this->getValue()); } - return $submit_button.$this->cancelButton; + return hsprintf('%s%s', $submit_button, $this->cancelButton); } } diff --git a/src/view/form/control/AphrontFormToggleButtonsControl.php b/src/view/form/control/AphrontFormToggleButtonsControl.php index 43f83ed1b9..1c2fb3a673 100644 --- a/src/view/form/control/AphrontFormToggleButtonsControl.php +++ b/src/view/form/control/AphrontFormToggleButtonsControl.php @@ -46,7 +46,7 @@ final class AphrontFormToggleButtonsControl extends AphrontFormControl { $label); } - return implode('', $out); + return $out; } } diff --git a/src/view/form/control/PhabricatorRemarkupControl.php b/src/view/form/control/PhabricatorRemarkupControl.php index 42b5d143b8..e34f090fcb 100644 --- a/src/view/form/control/PhabricatorRemarkupControl.php +++ b/src/view/form/control/PhabricatorRemarkupControl.php @@ -2,10 +2,12 @@ final class PhabricatorRemarkupControl extends AphrontFormTextAreaControl { private $disableMacro = false; + public function setDisableMacros($disable) { $this->disableMacro = $disable; return $this; } + protected function renderInput() { $id = $this->getID(); if (!$id) {