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
2025-01-22 20:51:10 +01:00 · 2013-02-05 14:30:29 -08:00 · 2013-02-05 14:30:29 -08:00 · 6bb7a282b1
commit 6bb7a282b1
parent be4662e667
20 changed files with 117 additions and 126 deletions
--- 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[] = '<ul>';
+      $error_description = array();
      foreach ($error_types as $error => $meaning) {
        $error_description[] = hsprintf(
          '<li><strong>%s:</strong> %s</li>',
          $error,
          $meaning);
      }
-      $error_description[] = '</ul>';
-      $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.";
    }
--- 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[] = '<tr class="column-labels">';
-    $table[] = '<th>'.pht('Example').'</th>';
-    $table[] = '<th>'.pht('Value').'</th>';
-    $table[] = '</tr>';
+    $table[] = hsprintf(
+      '<tr class="column-labels"><th>%s</th><th>%s</th></tr>',
+      pht('Example'),
+      pht('Value'));
    foreach ($examples as $example) {
      list($value, $description) = $example;

      if ($value === null) {
-        $value = '<em>'.pht('(empty)').'</em>';
+        $value = phutil_tag('em', array(), pht('(empty)'));
      } else {
-        $value = nl2br(phutil_escape_html($value));
+        $value = phutil_escape_html_newlines($value);
      }

-      $table[] = '<tr>';
-      $table[] = '<th>'.phutil_escape_html($description).'</th>';
-      $table[] = '<td>'.$value.'</td>';
-      $table[] = '</tr>';
+      $table[] = hsprintf(
+        '<tr><th>%s</th><td>%s</td></tr>',
+        $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[] = '<tr class="column-labels">';
-    $table[] = '<th>'.pht('Source').'</th>';
-    $table[] = '<th>'.pht('Value').'</th>';
-    $table[] = '</tr>';
+    $table[] = hsprintf(
+      '<tr class="column-labels"><th>%s</th><th>%s</th></tr>',
+      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 = '<em>'.pht('(empty)').'</em>';
+        $value = phutil_tag('em', array(), pht('(empty)'));
      } else {
        $value = PhabricatorConfigJSON::prettyPrintJSON(
          $value[$option->getKey()]);
      }

-      $table[] = '<tr>';
-      $table[] = '<th>'.phutil_escape_html($source->getName()).'</th>';
-      $table[] = '<td>'.$value.'</td>';
-      $table[] = '</tr>';
+      $table[] = hsprintf(
+        '<tr><th>%s</th><td>%s</td></tr>',
+        $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);
  }

 }
--- 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[] = '<optgroup label="Create New Revision">';
-      $select[] = '<option value="">'.
-                    pht('Create a new Revision...').
-                  '</option>';
-      $select[] = '</optgroup>';
+      $select[] = hsprintf('<optgroup label="%s">', pht('Create New Revision'));
+      $select[] = hsprintf(
+        '<option value="">%s</option>',
+        pht('Create a new Revision...'));
+      $select[] = hsprintf('</optgroup>');

      $revision_data = new DifferentialRevisionListData(
        DifferentialRevisionListData::QUERY_OPEN_OWNED,
@ -50,7 +50,9 @@ final class DifferentialDiffViewController extends DifferentialController {
      $revisions = $revision_data->loadRevisions();

      if ($revisions) {
-        $select[] = '<optgroup label="'.pht('Update Existing Revision').'">';
+        $select[] = hsprintf(
+          '<optgroup label="%s">',
+          pht('Update Existing Revision'));
        foreach ($revisions as $revision) {
          $select[] = phutil_tag(
            'option',
@ -59,13 +61,13 @@ final class DifferentialDiffViewController extends DifferentialController {
            ),
            $revision->getTitle());
        }
-        $select[] = '</optgroup>';
+        $select[] = hsprintf('</optgroup>');
      }

-      $select =
-        '<select name="revisionID">'.
-        implode("\n", $select).
-        '</select>';
+      $select = phutil_tag(
+        'select',
+        array('name' => 'revisionID'),
+        $select);

      $action_form = new AphrontFormView();
      $action_form
--- 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(
-        '<br /><br />'.
+      ->setValue(hsprintf(
+        '<br /><br /><strong>%s</strong> %s<br /><br />',
+        pht('Drag and Drop:'),
        pht(
-          '<strong>Drag and Drop:</strong> You can also upload files by '.
-          'dragging and dropping them from your desktop onto this page or '.
-          'the Phabricator home page.').
-        '<br /><br />');
+          '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)
--- 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 <strong>${rule_type_name}</strong> rule triggers for " .
-            "<strong>${content_type_name}</strong>."))
+          ->setValue(hsprintf(
+            "This <strong>%s</strong> rule triggers for <strong>%s</strong>.",
+            $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(
--- 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('<br />', $file_display);
+      $file_display = mpull($files, 'getName');
+      $file_display = array_interleave(phutil_tag('br'), $file_display);

      $form->appendChild(
        id(new AphrontFormMarkupControl())
--- 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();
--- 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);
  }

 }
--- 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(
-          '<pre class="PhabricatorMonospaced">'.
-          phutil_escape_html($example_string).
-          '</pre>'))
+        ->setValue(phutil_tag(
+          'pre',
+          array('class' => 'PhabricatorMonospaced'),
+          $example_string)))
      ->appendChild(
        id(new AphrontFormRadioButtonControl())
        ->setLabel('Monospaced Textareas')
--- 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()) {
--- 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[] =
-        '<tr>'.
-          '<td>'.$checkbox.'</td>'.
-          '<th>'.$label.'</th>'.
-        '</tr>';
+      $rows[] = hsprintf(
+        '<tr><td>%s</td><th>%s</th></tr>',
+        $checkbox,
+        $label);
    }
-    return
-      '<table class="aphront-form-control-checkbox-layout">'.
-        implode("\n", $rows).
-      '</table>';
+    return phutil_tag(
+      'table',
+      array('class' => 'aphront-form-control-checkbox-layout'),
+      $rows);
  }

 }
--- 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 class="aphront-form-label">'.
-          phutil_escape_html($this->getLabel()).
-        '</label>';
+      $label = phutil_tag(
+        'label',
+        array('class' => 'aphront-form-label'),
+        $this->getLabel());
    } else {
      $label = null;
      $custom_class .= ' aphront-form-control-nolabel';
    }

-    $input =
-      '<div class="aphront-form-input">'.
-        $this->renderInput().
-      '</div>';
+    $input = phutil_tag(
+      'div',
+      array('class' => 'aphront-form-input'),
+      $this->renderInput());

    if (strlen($this->getError())) {
      $error = $this->getError();
      if ($error === true) {
-        $error =
-          '<div class="aphront-form-error aphront-form-required">'.
-            'Required'.
-          '</div>';
+        $error = phutil_tag(
+          'div',
+          array('class' => 'aphront-form-error aphront-form-required'),
+          'Required');
      } else {
-        $error =
-          '<div class="aphront-form-error">'.
-            phutil_escape_html($error).
-          '</div>';
+        $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.
-      '<div style="clear: both;"></div>');
+        // TODO: Remove this once the redesign finishes up.
+        phutil_tag('div', array('style' => 'clear: both;'), ''),
+      ));
  }
 }
--- 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 '<hr />';
+    return phutil_tag('hr');
  }

 }
--- 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<div style="clear: both;">%s%s</div>',
      phutil_tag(
        'input',
        array(
          'type'  => 'file',
          'name'  => $this->getName(),
-        )).
-      '<div style="clear: both;">'.
+        )),
      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').
-      '</div>';
+        'Use Default Image instead'));
  }

 }
--- 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 .=
-          '<div class="aphront-form-radio-caption">'.
-            phutil_escape_html($button['caption']).
-          '</div>';
+        $label = hsprintf(
+          '%s<div class="aphront-form-radio-caption">%s</div>',
+          $label,
+          $button['caption']);
      }
-      $rows[] =
-        '<tr>'.
-          '<td>'.$radio.'</td>'.
-          '<th>'.$label.'</th>'.
-        '</tr>';
+      $rows[] = hsprintf(
+        '<tr><td>%s</td><th>%s</th></tr>',
+        $radio,
+        $label);
    }

-    return
-      '<table class="aphront-form-control-radio-layout">'.
-        implode("\n", $rows).
-      '</table>';
+    return phutil_tag(
+      'table',
+      array('class' => 'aphront-form-control-radio-layout'),
+      $rows);
  }

 }
--- 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));
  }

 }
--- 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();
  }

 }
--- 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);
  }

 }
--- 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;
  }

 }
--- 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) {