Kill most of phutil_escape_html()

Summary: This resolves lots of double escaping. We changed most of `phutil_render_tag(, , $s)` to `phutil_tag(, , $s)` which means that `$s` is now auto-escaped. Also `pht()` auto escapes if it gets `PhutilSafeHTML`. Test Plan: None. Reviewers: epriestley Reviewed By: epriestley CC: aran, Korvin Maniphest Tasks: T2432 Differential Revision: https://secure.phabricator.com/D4889
2024-12-24 14:30:56 +01:00 · 2013-02-09 14:43:10 -08:00 · 2013-02-09 14:43:10 -08:00 · a22ef4e9b4
commit a22ef4e9b4
parent 9b8da73765
24 changed files with 127 additions and 101 deletions
--- a/src/aphront/console/plugin/DarkConsoleErrorLogPlugin.php
+++ b/src/aphront/console/plugin/DarkConsoleErrorLogPlugin.php
@ -50,11 +50,11 @@ final class DarkConsoleErrorLogPlugin extends DarkConsolePlugin {
        $row['str'].' at ['.basename($file).':'.$line.']');
      $rows[] = array($tag);

-      $details .=
-        '<div class="dark-console-panel-error-details" id="row-details-'.
-        $index.'">'.
-        phutil_escape_html($row['details'])."\n".
-        'Stack trace:'."\n";
+      $details .= hsprintf(
+        '<div class="dark-console-panel-error-details" id="row-details-%s">'.
+        "%s\nStack trace:\n",
+        $index,
+        $row['details']);

      foreach ($row['trace'] as $key => $entry) {
        $line = '';
--- a/src/applications/auth/controller/PhabricatorLoginController.php
+++ b/src/applications/auth/controller/PhabricatorLoginController.php
@ -247,8 +247,7 @@ final class PhabricatorLoginController
        $title = pht("Login or Register with %s", $provider_name);
        $body = pht('Login or register for Phabricator using your %s account.',
          $provider_name);
-        $button = pht("Login or Register with %s",
-          phutil_escape_html($provider_name));
+        $button = pht("Login or Register with %s", $provider_name);
      } else {
        $title = pht("Login with %s", $provider_name);
        $body = hsprintf(
@ -259,7 +258,7 @@ final class PhabricatorLoginController
          pht(
            'You can not use %s to register a new account.',
            $provider_name));
-        $button = pht("Log in with %s", phutil_escape_html($provider_name));
+        $button = pht("Log in with %s", $provider_name);
      }

      $auth_form = new AphrontFormView();
--- a/src/applications/calendar/controller/PhabricatorCalendarViewStatusController.php
+++ b/src/applications/calendar/controller/PhabricatorCalendarViewStatusController.php
@ -94,7 +94,7 @@ final class PhabricatorCalendarViewStatusController
    } else {
      $no_data =
        pht('%s does not have any upcoming status events.',
-            phutil_escape_html($this->getHandle($this->phid)->getName()));
+            $this->getHandle($this->phid)->getName());
    }
    return $no_data;
  }
@ -115,7 +115,7 @@ final class PhabricatorCalendarViewStatusController
    } else {
      $page_title = pht(
        'Upcoming Statuses for %s',
-        phutil_escape_html($this->getHandle($this->phid)->getName())
+        $this->getHandle($this->phid)->getName()
      );
    }
    return $page_title;
--- a/src/applications/calendar/view/AphrontCalendarMonthView.php
+++ b/src/applications/calendar/view/AphrontCalendarMonthView.php
@ -100,11 +100,14 @@ final class AphrontCalendarMonthView extends AphrontView {

      $holiday_markup = null;
      if ($holiday) {
-        $name = phutil_escape_html($holiday->getName());
-        $holiday_markup =
-          '<div class="aphront-calendar-holiday" title="'.$name.'">'.
-            $name.
-          '</div>';
+        $name = $holiday->getName();
+        $holiday_markup = phutil_tag(
+          'div',
+          array(
+            'class' => 'aphront-calendar-holiday',
+            'title' => $name,
+          ),
+          $name);
      }

      $markup[] =
--- a/src/applications/conpherence/storage/ConpherenceTransaction.php
+++ b/src/applications/conpherence/storage/ConpherenceTransaction.php
@ -50,18 +50,18 @@ final class ConpherenceTransaction extends PhabricatorApplicationTransaction {
          $title = pht(
            '%s renamed this conpherence from "%s" to "%s".',
            $this->renderHandleLink($author_phid),
-            phutil_escape_html($old),
-            phutil_escape_html($new));
+            $old,
+            $new);
        } else if ($old) {
          $title = pht(
            '%s deleted the conpherence name "%s".',
            $this->renderHandleLink($author_phid),
-            phutil_escape_html($old));
+            $old);
        } else {
          $title = pht(
            '%s named this conpherence "%s".',
            $this->renderHandleLink($author_phid),
-            phutil_escape_html($new));
+            $new);
        }
        return $title;
      case ConpherenceTransactionType::TYPE_FILES:
--- a/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
@ -143,8 +143,7 @@ final class DifferentialReviewersFieldSpecification
      if ($other_reviewers) {
        $names = array();
        foreach ($other_reviewers as $reviewer => $_) {
-          $names[] = phutil_escape_html(
-            $this->getHandle($reviewer)->getLinkName());
+          $names[] = $this->getHandle($reviewer)->getLinkName();
        }
        $suffix = javelin_tag(
          'abbr',
--- a/src/applications/differential/view/DifferentialDiffTableOfContentsView.php
+++ b/src/applications/differential/view/DifferentialDiffTableOfContentsView.php
@ -94,22 +94,20 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
            $meta[] = pht('Copied to multiple locations:');
          }
          foreach ($away as $path) {
-            $meta[] = phutil_escape_html($path);
+            $meta[] = $path;
          }
-          $meta = implode('<br />', $meta);
+          $meta = phutil_implode_html(phutil_tag('br'), $meta);
        } else {
          if ($type == DifferentialChangeType::TYPE_MOVE_AWAY) {
-            $meta = pht('Moved to %s', phutil_escape_html(reset($away)));
+            $meta = pht('Moved to %s', reset($away));
          } else {
-            $meta = pht('Copied to %s', phutil_escape_html(reset($away)));
+            $meta = pht('Copied to %s', reset($away));
          }
        }
      } else if ($type == DifferentialChangeType::TYPE_MOVE_HERE) {
-        $meta = pht('Moved from %s',
-          phutil_escape_html($changeset->getOldFile()));
+        $meta = pht('Moved from %s', $changeset->getOldFile());
      } else if ($type == DifferentialChangeType::TYPE_COPY_HERE) {
-        $meta = pht('Copied from %s',
-          phutil_escape_html($changeset->getOldFile()));
+        $meta = pht('Copied from %s', $changeset->getOldFile());
      } else {
        $meta = null;
      }
@ -162,11 +160,12 @@ final class DifferentialDiffTableOfContentsView extends AphrontView {
          '<td class="differential-toc-mcov">'.$mcov.'</td>'.
        '</tr>';
      if ($meta) {
-        $rows[] =
+        $rows[] = hsprintf(
          '<tr>'.
            '<td colspan="3"></td>'.
-            '<td class="differential-toc-meta">'.$meta.'</td>'.
-          '</tr>';
+            '<td class="differential-toc-meta">%s</td>'.
+          '</tr>',
+          $meta);
      }
      if ($this->diff && $this->repository) {
        $paths[] =
--- a/src/applications/differential/view/DifferentialRevisionCommentView.php
+++ b/src/applications/differential/view/DifferentialRevisionCommentView.php
@ -116,19 +116,22 @@ final class DifferentialRevisionCommentView extends AphrontView {
      array());

    $verb = DifferentialAction::getActionPastTenseVerb($comment->getAction());
-    $verb = phutil_escape_html($verb);

    $actions = array();
    // TODO: i18n
    switch ($comment->getAction()) {
      case DifferentialAction::ACTION_ADDCCS:
-        $actions[] = "{$author_link} added CCs: ".
-          $this->renderHandleList($added_ccs).".";
+        $actions[] = hsprintf(
+          "%s added CCs: %s.",
+          $author_link,
+          $this->renderHandleList($added_ccs));
        $added_ccs = null;
        break;
      case DifferentialAction::ACTION_ADDREVIEWERS:
-        $actions[] = "{$author_link} added reviewers: ".
-          $this->renderHandleList($added_reviewers).".";
+        $actions[] = hsprintf(
+          "%s added reviewers: %s.",
+          $author_link,
+          $this->renderHandleList($added_reviewers));
        $added_reviewers = null;
        break;
      case DifferentialAction::ACTION_UPDATE:
@ -140,33 +143,48 @@ final class DifferentialRevisionCommentView extends AphrontView {
              'href' => '/D'.$comment->getRevisionID().'?id='.$diff_id,
            ),
            'Diff #'.$diff_id);
-          $actions[] = "{$author_link} updated this revision to {$diff_link}.";
+          $actions[] = hsprintf(
+            "%s updated this revision to %s.",
+            $author_link,
+            $diff_link);
        } else {
-          $actions[] = "{$author_link} {$verb} this revision.";
+          $actions[] = hsprintf(
+            "%s %s this revision.",
+            $author_link,
+            $verb);
        }
        break;
      default:
-        $actions[] = "{$author_link} {$verb} this revision.";
+        $actions[] = hsprintf(
+          "%s %s this revision.",
+          $author_link,
+          $verb);
        break;
    }

    if ($added_reviewers) {
-      $actions[] = "{$author_link} added reviewers: ".
-        $this->renderHandleList($added_reviewers).".";
+      $actions[] = hsprintf(
+        "%s added reviewers: %s.",
+        $author_link,
+        $this->renderHandleList($added_reviewers));
    }

    if ($removed_reviewers) {
-      $actions[] = "{$author_link} removed reviewers: ".
-        $this->renderHandleList($removed_reviewers).".";
+      $actions[] = hsprintf(
+        "%s removed reviewers: %s.",
+        $author_link,
+        $this->renderHandleList($removed_reviewers));
    }

    if ($added_ccs) {
-      $actions[] = "{$author_link} added CCs: ".
-        $this->renderHandleList($added_ccs).".";
+      $actions[] = hsprintf(
+        "%s added CCs: %s.",
+        $author_link,
+        $this->renderHandleList($added_ccs));
    }

    foreach ($actions as $key => $action) {
-      $actions[$key] = '<div>'.$action.'</div>';
+      $actions[$key] = phutil_tag('div', array(), $action);
    }

    $xaction_view = id(new PhabricatorTransactionView())
@ -205,7 +223,7 @@ final class DifferentialRevisionCommentView extends AphrontView {
    foreach ($phids as $phid) {
      $result[] = $this->handles[$phid]->renderLink();
    }
-    return implode(', ', $result);
+    return phutil_implode_html(', ', $result);
  }

  private function renderInlineComments() {
--- a/src/applications/diffusion/view/DiffusionCommentView.php
+++ b/src/applications/diffusion/view/DiffusionCommentView.php
@ -114,17 +114,19 @@ final class DiffusionCommentView extends AphrontView {
    $actions = array();
    if ($action == PhabricatorAuditActionConstants::ADD_CCS) {
      $rendered_ccs = $this->renderHandleList($added_ccs);
-      $actions[] = "{$author_link} added CCs: {$rendered_ccs}.";
+      $actions[] = hsprintf("%s added CCs: %s.", $author_link, $rendered_ccs);
    } else if ($action == PhabricatorAuditActionConstants::ADD_AUDITORS) {
      $rendered_auditors = $this->renderHandleList($added_auditors);
-      $actions[] = "{$author_link} added auditors: ".
-        "{$rendered_auditors}.";
+      $actions[] = hsprintf(
+        "%s added auditors: %s.",
+        $author_link,
+        $rendered_auditors);
    } else {
-      $actions[] = "{$author_link} ".phutil_escape_html($verb)." this commit.";
+      $actions[] = hsprintf("%s %s this commit.", $author_link, $verb);
    }

    foreach ($actions as $key => $action) {
-      $actions[$key] = '<div>'.$action.'</div>';
+      $actions[$key] = phutil_tag('div', array(), $action);
    }

    return $actions;
@ -186,7 +188,7 @@ final class DiffusionCommentView extends AphrontView {
    foreach ($phids as $phid) {
      $result[] = $this->handles[$phid]->renderLink();
    }
-    return implode(', ', $result);
+    return phutil_implode_html(', ', $result);
  }

  private function renderClasses() {
--- a/src/applications/feed/story/PhabricatorFeedStoryCommit.php
+++ b/src/applications/feed/story/PhabricatorFeedStoryCommit.php
@ -19,14 +19,14 @@ final class PhabricatorFeedStoryCommit extends PhabricatorFeedStory {
    if ($data->getValue('authorPHID')) {
      $author = $this->linkTo($data->getValue('authorPHID'));
    } else {
-      $author = phutil_escape_html($data->getValue('authorName'));
+      $author = $data->getValue('authorName');
    }

    $committer = null;
    if ($data->getValue('committerPHID')) {
      $committer = $this->linkTo($data->getValue('committerPHID'));
    } else if ($data->getValue('committerName')) {
-      $committer = phutil_escape_html($data->getValue('committerName'));
+      $committer = $data->getValue('committerName');
    }

    $commit = $this->linkTo($data->getValue('commitPHID'));
@ -37,9 +37,16 @@ final class PhabricatorFeedStoryCommit extends PhabricatorFeedStory {
    }

    if ($author) {
-      $title = "{$committer} committed {$commit} (authored by {$author})";
+      $title = hsprintf(
+        "%s committed %s (authored by %s)",
+        $committer,
+        $commit,
+        $author);
    } else {
-      $title = "{$committer} committed {$commit}";
+      $title = hsprintf(
+        "%s committed %s",
+        $committer,
+        $commit);
    }

    $view = new PhabricatorFeedStoryView();
--- a/src/applications/flag/events/PhabricatorFlagsUIEventListener.php
+++ b/src/applications/flag/events/PhabricatorFlagsUIEventListener.php
@ -31,7 +31,7 @@ final class PhabricatorFlagsUIEventListener extends PhutilEventListener {
      $flag_action = id(new PhabricatorActionView())
        ->setWorkflow(true)
        ->setHref('/flag/delete/'.$flag->getID().'/')
-        ->setName(phutil_escape_html('Remove '.$color.' Flag'))
+        ->setName('Remove '.$color.' Flag')
        ->setIcon('flag-'.$flag->getColor());
    } else {
      $flag_action = id(new PhabricatorActionView())
--- a/src/applications/macro/storage/PhabricatorMacroTransaction.php
+++ b/src/applications/macro/storage/PhabricatorMacroTransaction.php
@ -64,8 +64,8 @@ final class PhabricatorMacroTransaction
        return pht(
          '%s renamed this macro from "%s" to "%s".',
          $this->renderHandleLink($author_phid),
-          phutil_escape_html($old),
-          phutil_escape_html($new));
+          $old,
+          $new);
        break;
      case PhabricatorMacroTransactionType::TYPE_DISABLED:
        if ($new) {
@ -109,8 +109,8 @@ final class PhabricatorMacroTransaction
          '%s renamed %s from "%s" to "%s".',
          $this->renderHandleLink($author_phid),
          $this->renderHandleLink($object_phid),
-          phutil_escape_html($old),
-          phutil_escape_html($new));
+          $old,
+          $new);
      case PhabricatorMacroTransactionType::TYPE_DISABLED:
        if ($new) {
          return pht(
--- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php
+++ b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldDefaultSpecification.php
@ -152,13 +152,13 @@ class ManiphestAuxiliaryFieldDefaultSpecification
    switch ($this->getFieldType()) {
      case self::TYPE_BOOL:
        if ($this->getValue()) {
-          return phutil_escape_html($this->getCheckboxValue());
+          return $this->getCheckboxValue();
        } else {
          return null;
        }
      case self::TYPE_SELECT:
        $display = idx($this->getSelectOptions(), $this->getValue());
-        return phutil_escape_html($display);
+        return $display;
    }
    return parent::renderForDetailView();
  }
--- a/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php
+++ b/src/applications/maniphest/auxiliaryfield/ManiphestAuxiliaryFieldSpecification.php
@ -71,7 +71,7 @@ abstract class ManiphestAuxiliaryFieldSpecification {
  }

  public function renderForDetailView() {
-    return phutil_escape_html($this->getValue());
+    return $this->getValue();
  }


--- a/src/applications/paste/controller/PhabricatorPasteListController.php
+++ b/src/applications/paste/controller/PhabricatorPasteListController.php
@ -109,7 +109,7 @@ final class PhabricatorPasteListController extends PhabricatorPasteController {
      $lang_name = $paste->getLanguage();
      if ($lang_name) {
        $lang_name = idx($lang_map, $lang_name, $lang_name);
-        $item->addIcon('none', phutil_escape_html($lang_name));
+        $item->addIcon('none', $lang_name);
      }

      $list->addItem($item);
--- a/src/applications/pholio/storage/PholioTransaction.php
+++ b/src/applications/pholio/storage/PholioTransaction.php
@ -44,15 +44,15 @@ final class PholioTransaction extends PhabricatorApplicationTransaction {
        return pht(
          '%s renamed this mock from "%s" to "%s".',
          $this->renderHandleLink($author_phid),
-          phutil_escape_html($old),
-          phutil_escape_html($new));
+          $old,
+          $new);
        break;
      case PholioTransactionType::TYPE_DESCRIPTION:
        return pht(
          '%s updated the description of this mock. '.
          'The old description was: %s',
          $this->renderHandleLink($author_phid),
-          phutil_escape_html($old));
+          $old);
    }

    return parent::getTitle();
--- a/src/applications/phpast/controller/PhabricatorXHPASTViewInputController.php
+++ b/src/applications/phpast/controller/PhabricatorXHPASTViewInputController.php
@ -5,7 +5,6 @@ final class PhabricatorXHPASTViewInputController

  public function processRequest() {
    $input = $this->getStorageTree()->getInput();
-    return $this->buildXHPASTViewPanelResponse(
-      phutil_escape_html($input));
+    return $this->buildXHPASTViewPanelResponse($input);
  }
 }
--- a/src/applications/phpast/controller/PhabricatorXHPASTViewPanelController.php
+++ b/src/applications/phpast/controller/PhabricatorXHPASTViewPanelController.php
@ -20,7 +20,7 @@ abstract class PhabricatorXHPASTViewPanelController
  }

  protected function buildXHPASTViewPanelResponse($content) {
-    $content =
+    $content = hsprintf(
      '<!DOCTYPE html>'.
      '<html>'.
        '<head>'.
@ -57,10 +57,9 @@ li span {

          </style>'.
        '</head>'.
-        '<body>'.
-          $content.
-        '</body>'.
-      '</html>';
+        '<body>%s</body>'.
+      '</html>',
+      $content);

    $response = new AphrontWebpageResponse();
    $response->setFrameable(true);
--- a/src/applications/phpast/controller/PhabricatorXHPASTViewStreamController.php
+++ b/src/applications/phpast/controller/PhabricatorXHPASTViewStreamController.php
@ -27,6 +27,7 @@ final class PhabricatorXHPASTViewStreamController
        $token->getValue());
    }

-    return $this->buildXHPASTViewPanelResponse(implode('', $tokens));
+    return $this->buildXHPASTViewPanelResponse(
+      phutil_implode_html('', $tokens));
  }
 }
--- a/src/applications/phpast/controller/PhabricatorXHPASTViewTreeController.php
+++ b/src/applications/phpast/controller/PhabricatorXHPASTViewTreeController.php
@ -12,7 +12,7 @@ final class PhabricatorXHPASTViewTreeController
      $input,
      array(0, $stdout, ''));

-    $tree = '<ul>'.$this->buildTree($tree->getRootNode()).'</ul>';
+    $tree = phutil_tag('ul', array(), $this->buildTree($tree->getRootNode()));
    return $this->buildXHPASTViewPanelResponse($tree);
  }

@ -27,19 +27,19 @@ final class PhabricatorXHPASTViewTreeController
    }

    $tree = array();
-    $tree[] =
-      '<li>'.
+    $tree[] = phutil_tag(
+      'li',
+      array(),
      phutil_tag(
        'span',
        array(
          'title' => $title,
        ),
-          $name).
-      '</li>';
+        $name));
    foreach ($root->getChildren() as $child) {
-      $tree[] = '<ul>'.$this->buildTree($child).'</ul>';
+      $tree[] = phutil_tag('ul', array(), $this->buildTree($child));
    }
-    return implode("\n", $tree);
+    return phutil_implode_html("\n", $tree);
  }

 }
--- a/src/applications/policy/filter/PhabricatorPolicy.php
+++ b/src/applications/policy/filter/PhabricatorPolicy.php
@ -84,7 +84,7 @@ final class PhabricatorPolicy {
        ),
        $this->getName());
    } else {
-      $desc = phutil_escape_html($this->getName());
+      $desc = $this->getName();
    }

    switch ($this->getType()) {
--- a/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php
+++ b/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php
@ -36,7 +36,7 @@ final class PhabricatorSubscriptionsUIEventListener
        ->setDisabled(true)
        ->setRenderAsForm(true)
        ->setHref('/subscriptions/add/'.$object->getPHID().'/')
-        ->setName(phutil_escape_html('Automatically Subscribed'))
+        ->setName('Automatically Subscribed')
        ->setIcon('subscribe-auto');
    } else {
      $subscribed = false;
@ -59,7 +59,7 @@ final class PhabricatorSubscriptionsUIEventListener
          ->setWorkflow(true)
          ->setRenderAsForm(true)
          ->setHref('/subscriptions/delete/'.$object->getPHID().'/')
-          ->setName(phutil_escape_html('Unsubscribe'))
+          ->setName('Unsubscribe')
          ->setIcon('subscribe-delete');
      } else {
        $sub_action = id(new PhabricatorActionView())
@ -67,7 +67,7 @@ final class PhabricatorSubscriptionsUIEventListener
          ->setWorkflow(true)
          ->setRenderAsForm(true)
          ->setHref('/subscriptions/add/'.$object->getPHID().'/')
-          ->setName(phutil_escape_html('Subscribe'))
+          ->setName('Subscribe')
          ->setIcon('subscribe-add');
      }

--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@ -134,7 +134,7 @@ abstract class PhabricatorApplicationTransaction
    if ($this->renderingTarget == self::TARGET_HTML) {
      return $this->getHandle($phid)->renderLink();
    } else {
-      return $this->getHandle($phid)->getName();
+      return hsprintf('%s', $this->getHandle($phid)->getName());
    }
  }

@ -143,7 +143,7 @@ abstract class PhabricatorApplicationTransaction
    foreach ($phids as $phid) {
      $links[] = $this->renderHandleLink($phid);
    }
-    return phutil_safe_html(implode(', ', $links));
+    return phutil_implode_html(', ', $links);
  }

  public function getIcon() {
@ -218,16 +218,16 @@ abstract class PhabricatorApplicationTransaction
          '%s changed the visibility of this %s from "%s" to "%s".',
          $this->renderHandleLink($author_phid),
          $this->getApplicationObjectTypeName(),
-          phutil_escape_html($old),
-          phutil_escape_html($new));
+          $old,
+          $new);
      case PhabricatorTransactions::TYPE_EDIT_POLICY:
        // TODO: Render human-readable.
        return pht(
          '%s changed the edit policy of this %s from "%s" to "%s".',
          $this->renderHandleLink($author_phid),
          $this->getApplicationObjectTypeName(),
-          phutil_escape_html($old),
-          phutil_escape_html($new));
+          $old,
+          $new);
      case PhabricatorTransactions::TYPE_SUBSCRIBERS:
        $add = array_diff($new, $old);
        $rem = array_diff($old, $new);
--- a/src/view/layout/PhabricatorSourceCodeView.php
+++ b/src/view/layout/PhabricatorSourceCodeView.php
@ -38,7 +38,7 @@ final class PhabricatorSourceCodeView extends AphrontView {
          ),
          pht('...'));
      } else {
-        $content_number = phutil_escape_html($line_number);
+        $content_number = $line_number;
        $content_line = "\xE2\x80\x8B".$line;
      }