From 64b52b9952dd1ea098e1daf4260506568f40bfc9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Nov 2018 02:38:09 -0800
Subject: [PATCH] Make SELECT construction in PolicyAwareQuery safer

Summary: Depends on D19784. Ref T13217. Reduce uses of unsafe `%Q` in SELECT construction.

Test Plan: This reduces the number of safety warnings when loading Phabricator home from ~900 to ~800.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13217

Differential Revision: https://secure.phabricator.com/D19785
---
 .../query/PhabricatorRepositoryQuery.php      |  4 ++--
 src/infrastructure/query/PhabricatorQuery.php | 21 ++++++-------------
 ...PhabricatorCursorPagedPolicyAwareQuery.php |  4 ++--
 3 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php
index 035693ff75..4175ffc2d5 100644
--- a/src/applications/repository/query/PhabricatorRepositoryQuery.php
+++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php
@@ -491,10 +491,10 @@ final class PhabricatorRepositoryQuery
   protected function buildSelectClauseParts(AphrontDatabaseConnection $conn) {
     $parts = parent::buildSelectClauseParts($conn);
 
-    $parts[] = 'r.*';
+    $parts[] = qsprintf($conn, 'r.*');
 
     if ($this->shouldJoinSummaryTable()) {
-      $parts[] = 's.*';
+      $parts[] = qsprintf($conn, 's.*');
     }
 
     return $parts;
diff --git a/src/infrastructure/query/PhabricatorQuery.php b/src/infrastructure/query/PhabricatorQuery.php
index 5893297dbc..1dfe14b6f2 100644
--- a/src/infrastructure/query/PhabricatorQuery.php
+++ b/src/infrastructure/query/PhabricatorQuery.php
@@ -41,25 +41,16 @@ abstract class PhabricatorQuery extends Phobject {
   /**
    * @task format
    */
-  protected function formatSelectClause(array $parts) {
+  protected function formatSelectClause(
+    AphrontDatabaseConnection $conn,
+    array $parts) {
+
     $parts = $this->flattenSubclause($parts);
     if (!$parts) {
-      throw new Exception(pht('Can not build empty select clause!'));
+      throw new Exception(pht('Can not build empty SELECT clause!'));
     }
 
-    return 'SELECT '.$this->formatSelectSubclause($parts);
-  }
-
-
-  /**
-   * @task format
-   */
-  protected function formatSelectSubclause(array $parts) {
-    $parts = $this->flattenSubclause($parts);
-    if (!$parts) {
-      return null;
-    }
-    return implode(', ', $parts);
+    return qsprintf($conn, 'SELECT %LQ', $parts);
   }
 
 
diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
index 1784a8ce17..cd101a61d9 100644
--- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
+++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php
@@ -277,7 +277,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
    */
   protected function buildSelectClause(AphrontDatabaseConnection $conn) {
     $parts = $this->buildSelectClauseParts($conn);
-    return $this->formatSelectClause($parts);
+    return $this->formatSelectClause($conn, $parts);
   }
 
 
@@ -291,7 +291,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     if ($alias) {
       $select[] = qsprintf($conn, '%T.*', $alias);
     } else {
-      $select[] = '*';
+      $select[] = qsprintf($conn, '*');
     }
 
     $select[] = $this->buildEdgeLogicSelectClause($conn);