From 9ebb5f8cda56a5ff057500265a5afc5a3e4f0a3b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 3 Apr 2017 08:35:29 -0700
Subject: [PATCH 01/28] Don't downgrade accepts on update (fix "sticky accept")

Summary:
Fixes T12496. Sticky accept was accidentally impacted by the "void" changes in D17566.

Instead, don't always downgrade all accepts/rejects: on update, we only want to downgrade accepts.

Test Plan:
  - With sticky accept off, updated an accepted revision: new state is "needs review".
  - With sticky accept on, updated an accepted revision: new state is "accepted" (sticky accept working correctly).
  - Did "reject" + "request review" to make sure that still works, worked fine.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12496

Differential Revision: https://secure.phabricator.com/D17605
---
 .../editor/DifferentialTransactionEditor.php       | 14 ++++++++++++--
 .../DifferentialRevisionVoidTransaction.php        |  6 +++---
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index bf7faa2700..f24bc7abda 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -339,12 +339,22 @@ final class DifferentialTransactionEditor
       }
     }
 
-    if ($downgrade_accepts || $downgrade_rejects) {
+    $downgrade = array();
+    if ($downgrade_accepts) {
+      $downgrade[] = DifferentialReviewerStatus::STATUS_ACCEPTED;
+    }
+
+    if ($downgrade_accepts) {
+      $downgrade[] = DifferentialReviewerStatus::STATUS_REJECTED;
+    }
+
+    if ($downgrade) {
       $void_type = DifferentialRevisionVoidTransaction::TRANSACTIONTYPE;
+
       $results[] = id(new DifferentialTransaction())
         ->setTransactionType($void_type)
         ->setIgnoreOnNoEffect(true)
-        ->setNewValue(true);
+        ->setNewValue($downgrade);
     }
 
     $is_commandeer = false;
diff --git a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php
index ae684d94fe..5073a9c464 100644
--- a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php
@@ -28,7 +28,7 @@ final class DifferentialRevisionVoidTransaction
           AND reviewerStatus IN (%Ls)',
       $table_name,
       $object->getPHID(),
-      $this->getVoidableStatuses());
+      $value);
 
     return ipull($rows, 'reviewerPHID');
   }
@@ -47,11 +47,11 @@ final class DifferentialRevisionVoidTransaction
       'UPDATE %T SET voidedPHID = %s
         WHERE revisionPHID = %s
           AND voidedPHID IS NULL
-          AND reviewerStatus IN (%Ls)',
+          AND reviewerPHID IN (%Ls)',
       $table_name,
       $this->getActingAsPHID(),
       $object->getPHID(),
-      $this->getVoidableStatuses());
+      $value);
   }
 
   public function shouldHide() {

From 5e447112184e48a57509a57efeea7c5c067fa754 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 05:53:39 -0700
Subject: [PATCH 02/28] Provide a missing feed transaction string for space
 creation

Summary:
Fixes T12502. This transaction probably should not be getting picked for feed rendering, but it currently does get selected in some cases.

This should probably be revisited eventually (e.g., when Maniphest moves to ModularTransactions) but just fix the brokenness for now.

Test Plan:
  - Created a task in a space.
  - Viewed feed.
  - Saw the story render with readable text.

{F4555747}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12502

Differential Revision: https://secure.phabricator.com/D17609
---
 .../PhabricatorApplicationTransaction.php     | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index 5e36e942ce..bf3c616df9 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -1149,12 +1149,20 @@ abstract class PhabricatorApplicationTransaction
           $this->renderHandleLink($author_phid),
           $this->renderHandleLink($object_phid));
       case PhabricatorTransactions::TYPE_SPACE:
-        return pht(
-          '%s shifted %s from the %s space to the %s space.',
-          $this->renderHandleLink($author_phid),
-          $this->renderHandleLink($object_phid),
-          $this->renderHandleLink($old),
-          $this->renderHandleLink($new));
+        if ($this->getIsCreateTransaction()) {
+          return pht(
+            '%s created %s in the %s space.',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($object_phid),
+            $this->renderHandleLink($new));
+        } else {
+          return pht(
+            '%s shifted %s from the %s space to the %s space.',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($object_phid),
+            $this->renderHandleLink($old),
+            $this->renderHandleLink($new));
+        }
       case PhabricatorTransactions::TYPE_EDGE:
         $new = ipull($new, 'dst');
         $old = ipull($old, 'dst');

From 8500f78e451c61079ba9ca4e4f27a087e7ab1403 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 07:43:20 -0700
Subject: [PATCH 03/28] Move Files to ModularTransactions

Summary: Ref T11357. A lot of file creation doesn't go through transactions, so we only actually have one real transaction type: editing a file name.

Test Plan:
Created and edited files.

{F4559287}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17610
---
 src/__phutil_library_map__.php                |  6 +-
 .../PhabricatorBadgesBadgeNameTransaction.php |  2 +-
 .../PhabricatorFileEditController.php         |  4 +-
 .../files/editor/PhabricatorFileEditor.php    | 67 ------------------
 .../storage/PhabricatorFileTransaction.php    | 69 +------------------
 .../__tests__/PhabricatorFileTestCase.php     |  2 +-
 .../PhabricatorFileNameTransaction.php        | 55 +++++++++++++++
 .../PhabricatorFileTransactionType.php        |  4 ++
 8 files changed, 71 insertions(+), 138 deletions(-)
 create mode 100644 src/applications/files/xaction/PhabricatorFileNameTransaction.php
 create mode 100644 src/applications/files/xaction/PhabricatorFileTransactionType.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 9ccc495e7a..14d84a9ae7 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2765,6 +2765,7 @@ phutil_register_library_map(array(
     'PhabricatorFileLightboxController' => 'applications/files/controller/PhabricatorFileLightboxController.php',
     'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php',
     'PhabricatorFileListController' => 'applications/files/controller/PhabricatorFileListController.php',
+    'PhabricatorFileNameTransaction' => 'applications/files/xaction/PhabricatorFileNameTransaction.php',
     'PhabricatorFileQuery' => 'applications/files/query/PhabricatorFileQuery.php',
     'PhabricatorFileROT13StorageFormat' => 'applications/files/format/PhabricatorFileROT13StorageFormat.php',
     'PhabricatorFileRawStorageFormat' => 'applications/files/format/PhabricatorFileRawStorageFormat.php',
@@ -2783,6 +2784,7 @@ phutil_register_library_map(array(
     'PhabricatorFileTransaction' => 'applications/files/storage/PhabricatorFileTransaction.php',
     'PhabricatorFileTransactionComment' => 'applications/files/storage/PhabricatorFileTransactionComment.php',
     'PhabricatorFileTransactionQuery' => 'applications/files/query/PhabricatorFileTransactionQuery.php',
+    'PhabricatorFileTransactionType' => 'applications/files/xaction/PhabricatorFileTransactionType.php',
     'PhabricatorFileTransform' => 'applications/files/transform/PhabricatorFileTransform.php',
     'PhabricatorFileTransformController' => 'applications/files/controller/PhabricatorFileTransformController.php',
     'PhabricatorFileTransformListController' => 'applications/files/controller/PhabricatorFileTransformListController.php',
@@ -7890,6 +7892,7 @@ phutil_register_library_map(array(
     'PhabricatorFileLightboxController' => 'PhabricatorFileController',
     'PhabricatorFileLinkView' => 'AphrontTagView',
     'PhabricatorFileListController' => 'PhabricatorFileController',
+    'PhabricatorFileNameTransaction' => 'PhabricatorFileTransactionType',
     'PhabricatorFileQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorFileROT13StorageFormat' => 'PhabricatorFileStorageFormat',
     'PhabricatorFileRawStorageFormat' => 'PhabricatorFileStorageFormat',
@@ -7905,9 +7908,10 @@ phutil_register_library_map(array(
     'PhabricatorFileTestCase' => 'PhabricatorTestCase',
     'PhabricatorFileTestDataGenerator' => 'PhabricatorTestDataGenerator',
     'PhabricatorFileThumbnailTransform' => 'PhabricatorFileImageTransform',
-    'PhabricatorFileTransaction' => 'PhabricatorApplicationTransaction',
+    'PhabricatorFileTransaction' => 'PhabricatorModularTransaction',
     'PhabricatorFileTransactionComment' => 'PhabricatorApplicationTransactionComment',
     'PhabricatorFileTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
+    'PhabricatorFileTransactionType' => 'PhabricatorModularTransactionType',
     'PhabricatorFileTransform' => 'Phobject',
     'PhabricatorFileTransformController' => 'PhabricatorFileController',
     'PhabricatorFileTransformListController' => 'PhabricatorFileController',
diff --git a/src/applications/badges/xaction/PhabricatorBadgesBadgeNameTransaction.php b/src/applications/badges/xaction/PhabricatorBadgesBadgeNameTransaction.php
index 1df7d89a70..3a609fedb2 100644
--- a/src/applications/badges/xaction/PhabricatorBadgesBadgeNameTransaction.php
+++ b/src/applications/badges/xaction/PhabricatorBadgesBadgeNameTransaction.php
@@ -43,7 +43,7 @@ final class PhabricatorBadgesBadgeNameTransaction
       $new_value = $xaction->getNewValue();
       $new_length = strlen($new_value);
       if ($new_length > $max_length) {
-        $errors[] = $this->newRequiredError(
+        $errors[] = $this->newInvalidError(
           pht('The name can be no longer than %s characters.',
           new PhutilNumber($max_length)));
       }
diff --git a/src/applications/files/controller/PhabricatorFileEditController.php b/src/applications/files/controller/PhabricatorFileEditController.php
index e1b34afd73..a1e7a16d80 100644
--- a/src/applications/files/controller/PhabricatorFileEditController.php
+++ b/src/applications/files/controller/PhabricatorFileEditController.php
@@ -31,7 +31,7 @@ final class PhabricatorFileEditController extends PhabricatorFileController {
       $file_name = $request->getStr('name');
       $errors = array();
 
-      $type_name = PhabricatorFileTransaction::TYPE_NAME;
+      $type_name = PhabricatorFileNameTransaction::TRANSACTIONTYPE;
 
       $xactions = array();
 
@@ -40,7 +40,7 @@ final class PhabricatorFileEditController extends PhabricatorFileController {
         ->setNewValue($can_view);
 
       $xactions[] = id(new PhabricatorFileTransaction())
-        ->setTransactionType(PhabricatorFileTransaction::TYPE_NAME)
+        ->setTransactionType($type_name)
         ->setNewValue($file_name);
 
       $editor = id(new PhabricatorFileEditor())
diff --git a/src/applications/files/editor/PhabricatorFileEditor.php b/src/applications/files/editor/PhabricatorFileEditor.php
index 9cd81a4c1e..28b781fb37 100644
--- a/src/applications/files/editor/PhabricatorFileEditor.php
+++ b/src/applications/files/editor/PhabricatorFileEditor.php
@@ -17,46 +17,9 @@ final class PhabricatorFileEditor
     $types[] = PhabricatorTransactions::TYPE_COMMENT;
     $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY;
 
-    $types[] = PhabricatorFileTransaction::TYPE_NAME;
-
     return $types;
   }
 
-  protected function getCustomTransactionOldValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorFileTransaction::TYPE_NAME:
-        return $object->getName();
-    }
-  }
-
-  protected function getCustomTransactionNewValue(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorFileTransaction::TYPE_NAME:
-        return $xaction->getNewValue();
-    }
-  }
-
-  protected function applyCustomInternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {
-
-    switch ($xaction->getTransactionType()) {
-      case PhabricatorFileTransaction::TYPE_NAME:
-        $object->setName($xaction->getNewValue());
-        break;
-    }
-  }
-
-  protected function applyCustomExternalTransaction(
-    PhabricatorLiskDAO $object,
-    PhabricatorApplicationTransaction $xaction) {}
-
   protected function shouldSendMail(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -111,34 +74,4 @@ final class PhabricatorFileEditor
     return false;
   }
 
-  protected function validateTransaction(
-    PhabricatorLiskDAO $object,
-    $type,
-    array $xactions) {
-
-    $errors = parent::validateTransaction($object, $type, $xactions);
-
-    switch ($type) {
-      case PhabricatorFileTransaction::TYPE_NAME:
-        $missing = $this->validateIsEmptyTextField(
-          $object->getName(),
-          $xactions);
-
-        if ($missing) {
-          $error = new PhabricatorApplicationTransactionValidationError(
-            $type,
-            pht('Required'),
-            pht('File name is required.'),
-            nonempty(last($xactions), null));
-
-          $error->setIsMissingFieldError(true);
-          $errors[] = $error;
-        }
-        break;
-    }
-
-    return $errors;
-  }
-
-
 }
diff --git a/src/applications/files/storage/PhabricatorFileTransaction.php b/src/applications/files/storage/PhabricatorFileTransaction.php
index 663dde28c7..3c72be6fd7 100644
--- a/src/applications/files/storage/PhabricatorFileTransaction.php
+++ b/src/applications/files/storage/PhabricatorFileTransaction.php
@@ -1,9 +1,7 @@
 <?php
 
 final class PhabricatorFileTransaction
-  extends PhabricatorApplicationTransaction {
-
-  const TYPE_NAME  = 'file:name';
+  extends PhabricatorModularTransaction {
 
   public function getApplicationName() {
     return 'file';
@@ -17,69 +15,8 @@ final class PhabricatorFileTransaction
     return new PhabricatorFileTransactionComment();
   }
 
-  public function getTitle() {
-    $author_phid = $this->getAuthorPHID();
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_NAME:
-        return pht(
-          '%s updated the name for this file from "%s" to "%s".',
-          $this->renderHandleLink($author_phid),
-          $old,
-          $new);
-        break;
-    }
-
-    return parent::getTitle();
+  public function getBaseTransactionClass() {
+    return 'PhabricatorFileTransactionType';
   }
 
-  public function getTitleForFeed() {
-    $author_phid = $this->getAuthorPHID();
-    $object_phid = $this->getObjectPHID();
-
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    $type = $this->getTransactionType();
-    switch ($type) {
-      case self::TYPE_NAME:
-        return pht(
-          '%s updated the name of %s from "%s" to "%s".',
-          $this->renderHandleLink($author_phid),
-          $this->renderHandleLink($object_phid),
-          $old,
-          $new);
-        break;
-      }
-
-    return parent::getTitleForFeed();
-  }
-
-  public function getIcon() {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_NAME:
-        return 'fa-pencil';
-    }
-
-    return parent::getIcon();
-  }
-
-
-  public function getColor() {
-    $old = $this->getOldValue();
-    $new = $this->getNewValue();
-
-    switch ($this->getTransactionType()) {
-      case self::TYPE_NAME:
-        return PhabricatorTransactions::COLOR_BLUE;
-    }
-
-    return parent::getColor();
-  }
 }
diff --git a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
index 8aa22c6578..f9090c9c83 100644
--- a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
+++ b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
@@ -32,7 +32,7 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase {
     // First, change the name: this should not scramble the secret.
     $xactions = array();
     $xactions[] = id(new PhabricatorFileTransaction())
-      ->setTransactionType(PhabricatorFileTransaction::TYPE_NAME)
+      ->setTransactionType(PhabricatorFileNameTransaction::TRANSACTIONTYPE)
       ->setNewValue('test.dat2');
 
     $engine = id(new PhabricatorFileEditor())
diff --git a/src/applications/files/xaction/PhabricatorFileNameTransaction.php b/src/applications/files/xaction/PhabricatorFileNameTransaction.php
new file mode 100644
index 0000000000..7eb9396b66
--- /dev/null
+++ b/src/applications/files/xaction/PhabricatorFileNameTransaction.php
@@ -0,0 +1,55 @@
+<?php
+
+final class PhabricatorFileNameTransaction
+  extends PhabricatorFileTransactionType {
+
+  const TRANSACTIONTYPE = 'file:name';
+
+  public function generateOldValue($object) {
+    return $object->getName();
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setName($value);
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s updated the name for this file from "%s" to "%s".',
+      $this->renderAuthor(),
+      $this->renderOldValue(),
+      $this->renderNewValue());
+  }
+
+  public function getTitleForFeed() {
+    return pht(
+      '%s updated the name of %s from "%s" to "%s".',
+      $this->renderAuthor(),
+      $this->renderObject(),
+      $this->renderOldValue(),
+      $this->renderNewValue());
+  }
+
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    if ($this->isEmptyTextTransaction($object->getName(), $xactions)) {
+      $errors[] = $this->newRequiredError(pht('Files must have a name.'));
+    }
+
+    $max_length = $object->getColumnMaximumByteLength('name');
+    foreach ($xactions as $xaction) {
+      $new_value = $xaction->getNewValue();
+      $new_length = strlen($new_value);
+      if ($new_length > $max_length) {
+        $errors[] = $this->newInvalidError(
+          pht(
+            'File names must not be longer than %s character(s).',
+            new PhutilNumber($max_length)));
+      }
+    }
+
+    return $errors;
+  }
+
+}
diff --git a/src/applications/files/xaction/PhabricatorFileTransactionType.php b/src/applications/files/xaction/PhabricatorFileTransactionType.php
new file mode 100644
index 0000000000..cc708ac541
--- /dev/null
+++ b/src/applications/files/xaction/PhabricatorFileTransactionType.php
@@ -0,0 +1,4 @@
+<?php
+
+abstract class PhabricatorFileTransactionType
+  extends PhabricatorModularTransactionType {}

From 260a08a1282d78957ddb27d14b5561f22f3c2d1b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 08:02:37 -0700
Subject: [PATCH 04/28] Move Files editing and commenting to EditEngine

Summary:
Ref T11357. This moves editing and commenting (but not creation) to EditEngine.

Since only the name is really editable, this is pretty straightforward.

Test Plan: Renamed files; commented on files.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17611
---
 src/__phutil_library_map__.php                |   4 +-
 .../PhabricatorFilesApplication.php           |   3 +-
 .../PhabricatorFileCommentController.php      |  62 ----------
 .../PhabricatorFileEditController.php         | 112 +-----------------
 .../PhabricatorFileInfoController.php         |  21 ++--
 .../editor/PhabricatorFileEditEngine.php      |  79 ++++++++++++
 .../files/storage/PhabricatorFile.php         |   3 +
 7 files changed, 98 insertions(+), 186 deletions(-)
 delete mode 100644 src/applications/files/controller/PhabricatorFileCommentController.php
 create mode 100644 src/applications/files/editor/PhabricatorFileEditEngine.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 14d84a9ae7..99f9810052 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2743,7 +2743,6 @@ phutil_register_library_map(array(
     'PhabricatorFileChunk' => 'applications/files/storage/PhabricatorFileChunk.php',
     'PhabricatorFileChunkIterator' => 'applications/files/engine/PhabricatorFileChunkIterator.php',
     'PhabricatorFileChunkQuery' => 'applications/files/query/PhabricatorFileChunkQuery.php',
-    'PhabricatorFileCommentController' => 'applications/files/controller/PhabricatorFileCommentController.php',
     'PhabricatorFileComposeController' => 'applications/files/controller/PhabricatorFileComposeController.php',
     'PhabricatorFileController' => 'applications/files/controller/PhabricatorFileController.php',
     'PhabricatorFileDAO' => 'applications/files/storage/PhabricatorFileDAO.php',
@@ -2751,6 +2750,7 @@ phutil_register_library_map(array(
     'PhabricatorFileDeleteController' => 'applications/files/controller/PhabricatorFileDeleteController.php',
     'PhabricatorFileDropUploadController' => 'applications/files/controller/PhabricatorFileDropUploadController.php',
     'PhabricatorFileEditController' => 'applications/files/controller/PhabricatorFileEditController.php',
+    'PhabricatorFileEditEngine' => 'applications/files/editor/PhabricatorFileEditEngine.php',
     'PhabricatorFileEditField' => 'applications/transactions/editfield/PhabricatorFileEditField.php',
     'PhabricatorFileEditor' => 'applications/files/editor/PhabricatorFileEditor.php',
     'PhabricatorFileExternalRequest' => 'applications/files/storage/PhabricatorFileExternalRequest.php',
@@ -7860,7 +7860,6 @@ phutil_register_library_map(array(
       'Iterator',
     ),
     'PhabricatorFileChunkQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
-    'PhabricatorFileCommentController' => 'PhabricatorFileController',
     'PhabricatorFileComposeController' => 'PhabricatorFileController',
     'PhabricatorFileController' => 'PhabricatorController',
     'PhabricatorFileDAO' => 'PhabricatorLiskDAO',
@@ -7868,6 +7867,7 @@ phutil_register_library_map(array(
     'PhabricatorFileDeleteController' => 'PhabricatorFileController',
     'PhabricatorFileDropUploadController' => 'PhabricatorFileController',
     'PhabricatorFileEditController' => 'PhabricatorFileController',
+    'PhabricatorFileEditEngine' => 'PhabricatorEditEngine',
     'PhabricatorFileEditField' => 'PhabricatorEditField',
     'PhabricatorFileEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorFileExternalRequest' => array(
diff --git a/src/applications/files/application/PhabricatorFilesApplication.php b/src/applications/files/application/PhabricatorFilesApplication.php
index 5733439fcf..1010818c19 100644
--- a/src/applications/files/application/PhabricatorFilesApplication.php
+++ b/src/applications/files/application/PhabricatorFilesApplication.php
@@ -78,7 +78,8 @@ final class PhabricatorFilesApplication extends PhabricatorApplication {
         'comment/(?P<id>[1-9]\d*)/' => 'PhabricatorFileCommentController',
         'thread/(?P<phid>[^/]+)/' => 'PhabricatorFileLightboxController',
         'delete/(?P<id>[1-9]\d*)/' => 'PhabricatorFileDeleteController',
-        'edit/(?P<id>[1-9]\d*)/' => 'PhabricatorFileEditController',
+        $this->getEditRoutePattern('edit/')
+          => 'PhabricatorFileEditController',
         'info/(?P<phid>[^/]+)/' => 'PhabricatorFileInfoController',
         'imageproxy/' => 'PhabricatorFileImageProxyController',
         'transforms/(?P<id>[1-9]\d*)/' =>
diff --git a/src/applications/files/controller/PhabricatorFileCommentController.php b/src/applications/files/controller/PhabricatorFileCommentController.php
deleted file mode 100644
index 11833ea8d8..0000000000
--- a/src/applications/files/controller/PhabricatorFileCommentController.php
+++ /dev/null
@@ -1,62 +0,0 @@
-<?php
-
-final class PhabricatorFileCommentController extends PhabricatorFileController {
-
-  public function handleRequest(AphrontRequest $request) {
-    $viewer = $request->getViewer();
-    $id = $request->getURIData('id');
-
-    if (!$request->isFormPost()) {
-      return new Aphront400Response();
-    }
-
-    $file = id(new PhabricatorFileQuery())
-      ->setViewer($viewer)
-      ->withIDs(array($id))
-      ->executeOne();
-    if (!$file) {
-      return new Aphront404Response();
-    }
-
-    $is_preview = $request->isPreviewRequest();
-    $draft = PhabricatorDraft::buildFromRequest($request);
-
-    $view_uri = $file->getInfoURI();
-
-    $xactions = array();
-    $xactions[] = id(new PhabricatorFileTransaction())
-      ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
-      ->attachComment(
-        id(new PhabricatorFileTransactionComment())
-          ->setContent($request->getStr('comment')));
-
-    $editor = id(new PhabricatorFileEditor())
-      ->setActor($viewer)
-      ->setContinueOnNoEffect($request->isContinueRequest())
-      ->setContentSourceFromRequest($request)
-      ->setIsPreview($is_preview);
-
-    try {
-      $xactions = $editor->applyTransactions($file, $xactions);
-    } catch (PhabricatorApplicationTransactionNoEffectException $ex) {
-      return id(new PhabricatorApplicationTransactionNoEffectResponse())
-        ->setCancelURI($view_uri)
-        ->setException($ex);
-    }
-
-    if ($draft) {
-      $draft->replaceOrDelete();
-    }
-
-    if ($request->isAjax() && $is_preview) {
-      return id(new PhabricatorApplicationTransactionResponse())
-        ->setViewer($viewer)
-        ->setTransactions($xactions)
-        ->setIsPreview($is_preview);
-    } else {
-      return id(new AphrontRedirectResponse())
-        ->setURI($view_uri);
-    }
-  }
-
-}
diff --git a/src/applications/files/controller/PhabricatorFileEditController.php b/src/applications/files/controller/PhabricatorFileEditController.php
index a1e7a16d80..ea07d72722 100644
--- a/src/applications/files/controller/PhabricatorFileEditController.php
+++ b/src/applications/files/controller/PhabricatorFileEditController.php
@@ -1,114 +1,12 @@
 <?php
 
-final class PhabricatorFileEditController extends PhabricatorFileController {
+final class PhabricatorFileEditController
+  extends PhabricatorFileController {
 
   public function handleRequest(AphrontRequest $request) {
-    $viewer = $request->getViewer();
-    $id = $request->getURIData('id');
-
-    $file = id(new PhabricatorFileQuery())
-      ->setViewer($viewer)
-      ->withIDs(array($id))
-      ->requireCapabilities(
-        array(
-          PhabricatorPolicyCapability::CAN_VIEW,
-          PhabricatorPolicyCapability::CAN_EDIT,
-        ))
-      ->executeOne();
-    if (!$file) {
-      return new Aphront404Response();
-    }
-
-    $title = pht('Edit File: %s', $file->getName());
-    $file_name = $file->getName();
-    $header_icon = 'fa-pencil';
-    $view_uri = '/'.$file->getMonogram();
-    $error_name = true;
-    $validation_exception = null;
-
-    if ($request->isFormPost()) {
-      $can_view = $request->getStr('canView');
-      $file_name = $request->getStr('name');
-      $errors = array();
-
-      $type_name = PhabricatorFileNameTransaction::TRANSACTIONTYPE;
-
-      $xactions = array();
-
-      $xactions[] = id(new PhabricatorFileTransaction())
-        ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY)
-        ->setNewValue($can_view);
-
-      $xactions[] = id(new PhabricatorFileTransaction())
-        ->setTransactionType($type_name)
-        ->setNewValue($file_name);
-
-      $editor = id(new PhabricatorFileEditor())
-        ->setActor($viewer)
-        ->setContentSourceFromRequest($request)
-        ->setContinueOnNoEffect(true);
-
-      try {
-        $editor->applyTransactions($file, $xactions);
-        return id(new AphrontRedirectResponse())->setURI($view_uri);
-      } catch (PhabricatorApplicationTransactionValidationException $ex) {
-        $validation_exception = $ex;
-        $error_name = $ex->getShortMessage($type_name);
-
-        $file->setViewPolicy($can_view);
-      }
-    }
-
-
-    $policies = id(new PhabricatorPolicyQuery())
-      ->setViewer($viewer)
-      ->setObject($file)
-      ->execute();
-
-    $form = id(new AphrontFormView())
-      ->setUser($viewer)
-       ->appendChild(
-        id(new AphrontFormTextControl())
-        ->setName('name')
-        ->setValue($file_name)
-        ->setLabel(pht('Name'))
-        ->setError($error_name))
-      ->appendChild(
-        id(new AphrontFormPolicyControl())
-          ->setUser($viewer)
-          ->setCapability(PhabricatorPolicyCapability::CAN_VIEW)
-          ->setPolicyObject($file)
-          ->setPolicies($policies)
-          ->setName('canView'))
-      ->appendChild(
-        id(new AphrontFormSubmitControl())
-          ->addCancelButton($view_uri)
-          ->setValue(pht('Save Changes')));
-
-    $crumbs = $this->buildApplicationCrumbs()
-      ->addTextCrumb($file->getMonogram(), $view_uri)
-      ->addTextCrumb(pht('Edit'))
-      ->setBorder(true);
-
-    $box = id(new PHUIObjectBoxView())
-      ->setHeaderText($title)
-      ->setValidationException($validation_exception)
-      ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY)
-      ->appendChild($form);
-
-    $header = id(new PHUIHeaderView())
-      ->setHeader($title)
-      ->setHeaderIcon($header_icon);
-
-    $view = id(new PHUITwoColumnView())
-      ->setHeader($header)
-      ->setFooter($box);
-
-    return $this->newPage()
-      ->setTitle($title)
-      ->setCrumbs($crumbs)
-      ->appendChild($view);
-
+    return id(new PhabricatorFileEditEngine())
+      ->setController($this)
+      ->buildResponse();
   }
 
 }
diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php
index 4ca3bbb72e..8e92423fbc 100644
--- a/src/applications/files/controller/PhabricatorFileInfoController.php
+++ b/src/applications/files/controller/PhabricatorFileInfoController.php
@@ -94,25 +94,18 @@ final class PhabricatorFileInfoController extends PhabricatorFileController {
       $file,
       new PhabricatorFileTransactionQuery());
 
-    $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business');
+    $comment_view = id(new PhabricatorFileEditEngine())
+      ->setViewer($viewer)
+      ->buildEditEngineCommentView($file);
 
-    $add_comment_header = $is_serious
-      ? pht('Add Comment')
-      : pht('Question File Integrity');
+    $monogram = $file->getMonogram();
 
-    $draft = PhabricatorDraft::newFromUserAndKey($viewer, $file->getPHID());
-
-    $add_comment_form = id(new PhabricatorApplicationTransactionCommentView())
-      ->setUser($viewer)
-      ->setObjectPHID($file->getPHID())
-      ->setDraft($draft)
-      ->setHeaderText($add_comment_header)
-      ->setAction($this->getApplicationURI('/comment/'.$file->getID().'/'))
-      ->setSubmitButtonName(pht('Add Comment'));
+    $timeline->setQuoteRef($monogram);
+    $comment_view->setTransactionTimeline($timeline);
 
     return array(
       $timeline,
-      $add_comment_form,
+      $comment_view,
     );
   }
 
diff --git a/src/applications/files/editor/PhabricatorFileEditEngine.php b/src/applications/files/editor/PhabricatorFileEditEngine.php
new file mode 100644
index 0000000000..04c4753bd5
--- /dev/null
+++ b/src/applications/files/editor/PhabricatorFileEditEngine.php
@@ -0,0 +1,79 @@
+<?php
+
+final class PhabricatorFileEditEngine
+  extends PhabricatorEditEngine {
+
+  const ENGINECONST = 'files.file';
+
+  public function getEngineName() {
+    return pht('Files');
+  }
+
+  protected function supportsEditEngineConfiguration() {
+    return false;
+  }
+
+  protected function getCreateNewObjectPolicy() {
+    // TODO: For now, this EditEngine can only edit objects, since there is
+    // a lot of complexity in dealing with file data during file creation.
+    return PhabricatorPolicies::POLICY_NOONE;
+  }
+
+  public function getSummaryHeader() {
+    return pht('Configure Files Forms');
+  }
+
+  public function getSummaryText() {
+    return pht('Configure creation and editing forms in Files.');
+  }
+
+  public function getEngineApplicationClass() {
+    return 'PhabricatorFilesApplication';
+  }
+
+  protected function newEditableObject() {
+    return PhabricatorFile::initializeNewFile();
+  }
+
+  protected function newObjectQuery() {
+    return new PhabricatorFileQuery();
+  }
+
+  protected function getObjectCreateTitleText($object) {
+    return pht('Create New File');
+  }
+
+  protected function getObjectEditTitleText($object) {
+    return pht('Edit File: %s', $object->getName());
+  }
+
+  protected function getObjectEditShortText($object) {
+    return $object->getMonogram();
+  }
+
+  protected function getObjectCreateShortText() {
+    return pht('Create File');
+  }
+
+  protected function getObjectName() {
+    return pht('File');
+  }
+
+  protected function getObjectViewURI($object) {
+    return $object->getURI();
+  }
+
+  protected function buildCustomEditFields($object) {
+    return array(
+      id(new PhabricatorTextEditField())
+        ->setKey('name')
+        ->setLabel(pht('Name'))
+        ->setTransactionType(PhabricatorFileNameTransaction::TRANSACTIONTYPE)
+        ->setDescription(pht('The name of the file.'))
+        ->setConduitDescription(pht('Rename the file.'))
+        ->setConduitTypeDescription(pht('New file name.'))
+        ->setValue($object->getName()),
+    );
+  }
+
+}
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index d301abceca..da7ad3f729 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -774,6 +774,9 @@ final class PhabricatorFile extends PhabricatorFileDAO
     return $format->newReadIterator($raw_iterator);
   }
 
+  public function getURI() {
+    return $this->getInfoURI();
+  }
 
   public function getViewURI() {
     if (!$this->getPHID()) {

From 2369fa38e1879134571f02b93961b98e19236bd9 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 08:48:05 -0700
Subject: [PATCH 05/28] Provide a modern ("v3") API for querying files
 ("file.search")

Summary: Ref T11357. Implements a modern `file.search` for files, and freezes `file.info`.

Test Plan: Ran `file.search` from the Conduit console.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17612
---
 src/__phutil_library_map__.php                |  3 ++
 .../conduit/FileInfoConduitAPIMethod.php      | 10 ++++++
 .../PhabricatorFileSearchConduitAPIMethod.php | 18 ++++++++++
 .../files/storage/PhabricatorFile.php         | 36 ++++++++++++++++++-
 4 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 src/applications/files/conduit/PhabricatorFileSearchConduitAPIMethod.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 99f9810052..f082c2fe22 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2770,6 +2770,7 @@ phutil_register_library_map(array(
     'PhabricatorFileROT13StorageFormat' => 'applications/files/format/PhabricatorFileROT13StorageFormat.php',
     'PhabricatorFileRawStorageFormat' => 'applications/files/format/PhabricatorFileRawStorageFormat.php',
     'PhabricatorFileSchemaSpec' => 'applications/files/storage/PhabricatorFileSchemaSpec.php',
+    'PhabricatorFileSearchConduitAPIMethod' => 'applications/files/conduit/PhabricatorFileSearchConduitAPIMethod.php',
     'PhabricatorFileSearchEngine' => 'applications/files/query/PhabricatorFileSearchEngine.php',
     'PhabricatorFileStorageBlob' => 'applications/files/storage/PhabricatorFileStorageBlob.php',
     'PhabricatorFileStorageConfigurationException' => 'applications/files/exception/PhabricatorFileStorageConfigurationException.php',
@@ -7847,6 +7848,7 @@ phutil_register_library_map(array(
       'PhabricatorFlaggableInterface',
       'PhabricatorPolicyInterface',
       'PhabricatorDestructibleInterface',
+      'PhabricatorConduitResultInterface',
     ),
     'PhabricatorFileAES256StorageFormat' => 'PhabricatorFileStorageFormat',
     'PhabricatorFileBundleLoader' => 'Phobject',
@@ -7897,6 +7899,7 @@ phutil_register_library_map(array(
     'PhabricatorFileROT13StorageFormat' => 'PhabricatorFileStorageFormat',
     'PhabricatorFileRawStorageFormat' => 'PhabricatorFileStorageFormat',
     'PhabricatorFileSchemaSpec' => 'PhabricatorConfigSchemaSpec',
+    'PhabricatorFileSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod',
     'PhabricatorFileSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'PhabricatorFileStorageBlob' => 'PhabricatorFileDAO',
     'PhabricatorFileStorageConfigurationException' => 'Exception',
diff --git a/src/applications/files/conduit/FileInfoConduitAPIMethod.php b/src/applications/files/conduit/FileInfoConduitAPIMethod.php
index 5f1bb6e936..f1c8f5941a 100644
--- a/src/applications/files/conduit/FileInfoConduitAPIMethod.php
+++ b/src/applications/files/conduit/FileInfoConduitAPIMethod.php
@@ -10,6 +10,16 @@ final class FileInfoConduitAPIMethod extends FileConduitAPIMethod {
     return pht('Get information about a file.');
   }
 
+  public function getMethodStatus() {
+    return self::METHOD_STATUS_FROZEN;
+  }
+
+  public function getMethodStatusDescription() {
+    return pht(
+      'This method is frozen and will eventually be deprecated. New code '.
+      'should use "file.search" instead.');
+  }
+
   protected function defineParamTypes() {
     return array(
       'phid' => 'optional phid',
diff --git a/src/applications/files/conduit/PhabricatorFileSearchConduitAPIMethod.php b/src/applications/files/conduit/PhabricatorFileSearchConduitAPIMethod.php
new file mode 100644
index 0000000000..0b6920d559
--- /dev/null
+++ b/src/applications/files/conduit/PhabricatorFileSearchConduitAPIMethod.php
@@ -0,0 +1,18 @@
+<?php
+
+final class PhabricatorFileSearchConduitAPIMethod
+  extends PhabricatorSearchEngineAPIMethod {
+
+  public function getAPIMethodName() {
+    return 'file.search';
+  }
+
+  public function newSearchEngine() {
+    return new PhabricatorFileSearchEngine();
+  }
+
+  public function getMethodSummary() {
+    return pht('Read information about files.');
+  }
+
+}
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index da7ad3f729..3cd734ba5e 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -24,7 +24,8 @@ final class PhabricatorFile extends PhabricatorFileDAO
     PhabricatorSubscribableInterface,
     PhabricatorFlaggableInterface,
     PhabricatorPolicyInterface,
-    PhabricatorDestructibleInterface {
+    PhabricatorDestructibleInterface,
+    PhabricatorConduitResultInterface {
 
   const METADATA_IMAGE_WIDTH  = 'width';
   const METADATA_IMAGE_HEIGHT = 'height';
@@ -1468,4 +1469,37 @@ final class PhabricatorFile extends PhabricatorFileDAO
     $this->saveTransaction();
   }
 
+
+/* -(  PhabricatorConduitResultInterface  )---------------------------------- */
+
+
+  public function getFieldSpecificationsForConduit() {
+    return array(
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('name')
+        ->setType('string')
+        ->setDescription(pht('The name of the file.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('dataURI')
+        ->setType('string')
+        ->setDescription(pht('Download URI for the file data.')),
+      id(new PhabricatorConduitSearchFieldSpecification())
+        ->setKey('size')
+        ->setType('int')
+        ->setDescription(pht('File size, in bytes.')),
+    );
+  }
+
+  public function getFieldValuesForConduit() {
+    return array(
+      'name' => $this->getName(),
+      'dataURI' => $this->getCDNURI(),
+      'size' => (int)$this->getByteSize(),
+    );
+  }
+
+  public function getConduitSearchAttachments() {
+    return array();
+  }
+
 }

From 2896da384cb7366fbcf41f8488ecea281393dfba Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 09:06:00 -0700
Subject: [PATCH 06/28] Only require POST to fetch file data if the viewer is
 logged in

Summary:
Ref T11357. In D17611, I added `file.search`, which includes a `"dataURI"`. Partly, this is building toward resolving T8348.

However, in some cases you can't GET this URI because of a security measure:

  - You have not configured `security.alternate-file-domain`.
  - The file isn't web-viewable.
  - (The request isn't an LFS request.)

The goal of this security mechanism is just to protect against session hijacking, so it's also safe to disable it if the viewer didn't present any credentials (since that means there's nothing to hijack). Add that exception, and reorganize the code a little bit.

Test Plan:
  - From the browser (with a session), tried to GET a binary data file. Got redirected.
  - Got a download with POST.
  - From the CLI (without a session), tried to GET a binary data file. Go a download.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17613
---
 .../PhabricatorFileDataController.php         | 22 ++++++++++++++-----
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php
index 5c2fc7bbda..43b5aa419f 100644
--- a/src/applications/files/controller/PhabricatorFileDataController.php
+++ b/src/applications/files/controller/PhabricatorFileDataController.php
@@ -84,18 +84,28 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
     if ($is_viewable && !$force_download) {
       $response->setMimeType($file->getViewableMimeType());
     } else {
-      if (!$request->isHTTPPost() && !$is_alternate_domain && !$is_lfs) {
-        // NOTE: Require POST to download files from the primary domain. We'd
-        // rather go full-bore and do a real CSRF check, but can't currently
-        // authenticate users on the file domain. This should blunt any
-        // attacks based on iframes, script tags, applet tags, etc., at least.
-        // Send the user to the "info" page if they're using some other method.
+      $is_public = !$viewer->isLoggedIn();
+      $is_post = $request->isHTTPPost();
 
+      // NOTE: Require POST to download files from the primary domain if the
+      // request includes credentials. The "Download File" links we generate
+      // in the web UI are forms which use POST to satisfy this requirement.
+
+      // The intent is to make attacks based on tags like "<iframe />" and
+      // "<script />" (which can issue GET requests, but can not easily issue
+      // POST requests) more difficult to execute.
+
+      // The best defense against these attacks is to use an alternate file
+      // domain, which is why we strongly recommend doing so.
+
+      $is_safe = ($is_alternate_domain || $is_lfs || $is_post || $is_public);
+      if (!$is_safe) {
         // This is marked as "external" because it is fully qualified.
         return id(new AphrontRedirectResponse())
           ->setIsExternal(true)
           ->setURI(PhabricatorEnv::getProductionURI($file->getBestURI()));
       }
+
       $response->setMimeType($file->getMimeType());
       $response->setDownload($file->getName());
     }

From 45b386596e62927abcb41a5d95d0e8c0eb5f923b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 11:01:43 -0700
Subject: [PATCH 07/28] Make the Files "TTL" API more structured

Summary:
Ref T11357. When creating a file, callers can currently specify a `ttl`. However, it isn't unambiguous what you're supposed to pass, and some callers get it wrong.

For example, to mean "this file expires in 60 minutes", you might pass either of these:

  - `time() + phutil_units('60 minutes in seconds')`
  - `phutil_units('60 minutes in seconds')`

The former means "60 minutes from now". The latter means "1 AM, January 1, 1970". In practice, because the GC normally runs only once every four hours (at least, until recently), and all the bad TTLs are cases where files are normally accessed immediately, these 1970 TTLs didn't cause any real problems.

Split `ttl` into `ttl.relative` and `ttl.absolute`, and make sure the values are sane. Then correct all callers, and simplify out the `time()` calls where possible to make switching to `PhabricatorTime` easier.

Test Plan:
- Generated an SSH keypair.
- Viewed a changeset.
- Viewed a raw diff.
- Viewed a commit's file data.
- Viewed a temporary file's details, saw expiration date and relative time.
- Ran unit tests.
- (Didn't really test Phragment.)

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17616
---
 ...habricatorAuthSSHKeyGenerateController.php |  2 +-
 .../DifferentialChangesetViewController.php   |  2 +-
 .../DifferentialRevisionViewController.php    |  2 +-
 .../query/DiffusionFileFutureQuery.php        |  4 +-
 .../files/PhabricatorImageTransformer.php     |  2 +-
 .../conduit/FileAllocateConduitAPIMethod.php  |  2 +-
 .../PhabricatorFileInfoController.php         | 12 ++++
 .../files/storage/PhabricatorFile.php         | 65 +++++++++++++++++--
 .../__tests__/PhabricatorFileTestCase.php     |  4 +-
 .../PhabricatorFileUploadSource.php           | 12 ++--
 .../PhragmentGetPatchConduitAPIMethod.php     |  2 +-
 .../controller/PhragmentPatchController.php   |  2 +-
 .../controller/PhragmentZIPController.php     |  2 +-
 13 files changed, 91 insertions(+), 22 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php
index 69d93548e5..070131b443 100644
--- a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php
+++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php
@@ -28,7 +28,7 @@ final class PhabricatorAuthSSHKeyGenerateController
         $private_key,
         array(
           'name' => $default_name.'.key',
-          'ttl' => time() + (60 * 10),
+          'ttl.relative' => phutil_units('10 minutes in seconds'),
           'viewPolicy' => $viewer->getPHID(),
         ));
 
diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php
index b5bfe7464d..c2bd4bf8de 100644
--- a/src/applications/differential/controller/DifferentialChangesetViewController.php
+++ b/src/applications/differential/controller/DifferentialChangesetViewController.php
@@ -357,7 +357,7 @@ final class DifferentialChangesetViewController extends DifferentialController {
         array(
           'name' => $changeset->getFilename(),
           'mime-type' => 'text/plain',
-          'ttl' => phutil_units('24 hours in seconds'),
+          'ttl.relative' => phutil_units('24 hours in seconds'),
           'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
         ));
 
diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index e7d0a1cc87..26ff22838f 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -893,7 +893,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $raw_diff,
       array(
         'name' => $file_name,
-        'ttl' => (60 * 60 * 24),
+        'ttl.relative' => phutil_units('24 hours in seconds'),
         'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
       ));
 
diff --git a/src/applications/diffusion/query/DiffusionFileFutureQuery.php b/src/applications/diffusion/query/DiffusionFileFutureQuery.php
index 8ce59028e2..250e4962da 100644
--- a/src/applications/diffusion/query/DiffusionFileFutureQuery.php
+++ b/src/applications/diffusion/query/DiffusionFileFutureQuery.php
@@ -93,7 +93,7 @@ abstract class DiffusionFileFutureQuery
     $drequest = $this->getRequest();
 
     $name = basename($drequest->getPath());
-    $ttl = PhabricatorTime::getNow() + phutil_units('48 hours in seconds');
+    $relative_ttl = phutil_units('48 hours in seconds');
 
     try {
       $threshold = PhabricatorFileStorageEngine::getChunkThreshold();
@@ -101,7 +101,7 @@ abstract class DiffusionFileFutureQuery
 
       $source = id(new PhabricatorExecFutureFileUploadSource())
         ->setName($name)
-        ->setTTL($ttl)
+        ->setRelativeTTL($relative_ttl)
         ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE)
         ->setExecFuture($future);
 
diff --git a/src/applications/files/PhabricatorImageTransformer.php b/src/applications/files/PhabricatorImageTransformer.php
index 75e83c223d..df4243cfdd 100644
--- a/src/applications/files/PhabricatorImageTransformer.php
+++ b/src/applications/files/PhabricatorImageTransformer.php
@@ -15,7 +15,7 @@ final class PhabricatorImageTransformer extends Phobject {
       $image,
       array(
         'name' => 'meme-'.$file->getName(),
-        'ttl' => time() + 60 * 60 * 24,
+        'ttl.relative' => phutil_units('24 hours in seconds'),
         'canCDN' => true,
       ));
   }
diff --git a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php
index ff3a0c80da..ddc5fa14fe 100644
--- a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php
+++ b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php
@@ -42,7 +42,7 @@ final class FileAllocateConduitAPIMethod
 
     $ttl = $request->getValue('deleteAfterEpoch');
     if ($ttl) {
-      $properties['ttl'] = $ttl;
+      $properties['ttl.absolute'] = $ttl;
     }
 
     $file = null;
diff --git a/src/applications/files/controller/PhabricatorFileInfoController.php b/src/applications/files/controller/PhabricatorFileInfoController.php
index 8e92423fbc..7217f284b4 100644
--- a/src/applications/files/controller/PhabricatorFileInfoController.php
+++ b/src/applications/files/controller/PhabricatorFileInfoController.php
@@ -212,6 +212,18 @@ final class PhabricatorFileInfoController extends PhabricatorFileController {
       pht('Mime Type'),
       $file->getMimeType());
 
+    $ttl = $file->getTtl();
+    if ($ttl) {
+      $delta = $ttl - PhabricatorTime::getNow();
+
+      $finfo->addProperty(
+        pht('Expires'),
+        pht(
+          '%s (%s)',
+          phabricator_datetime($ttl, $viewer),
+          phutil_format_relative_time_detailed($delta)));
+    }
+
     $width = $file->getImageWidth();
     if ($width) {
       $finfo->addProperty(
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index 3cd734ba5e..4136f1fdf8 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -9,10 +9,13 @@
  *
  *   | name | Human readable filename.
  *   | authorPHID | User PHID of uploader.
- *   | ttl | Temporary file lifetime, in seconds.
+ *   | ttl.absolute | Temporary file lifetime as an epoch timestamp.
+ *   | ttl.relative | Temporary file lifetime, relative to now, in seconds.
  *   | viewPolicy | File visibility policy.
  *   | isExplicitUpload | Used to show users files they explicitly uploaded.
  *   | canCDN | Allows the file to be cached and delivered over a CDN.
+ *   | profile | Marks the file as a profile image.
+ *   | format | Internal encoding format.
  *   | mime-type | Optional, explicit file MIME type.
  *   | builtin | Optional filename, identifies this as a builtin.
  *
@@ -1110,7 +1113,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
       $params = array(
         'name' => $builtin->getBuiltinDisplayName(),
-        'ttl'  => time() + (60 * 60 * 24 * 7),
+        'ttl.relative' => phutil_units('7 days in seconds'),
         'canCDN' => true,
         'builtin' => $key,
       );
@@ -1280,14 +1283,68 @@ final class PhabricatorFile extends PhabricatorFileDAO
    * @return this
    */
   private function readPropertiesFromParameters(array $params) {
+    PhutilTypeSpec::checkMap(
+      $params,
+      array(
+        'name' => 'optional string',
+        'authorPHID' => 'optional string',
+        'ttl.relative' => 'optional int',
+        'ttl.absolute' => 'optional int',
+        'viewPolicy' => 'optional string',
+        'isExplicitUpload' => 'optional bool',
+        'canCDN' => 'optional bool',
+        'profile' => 'optional bool',
+        'format' => 'optional string|PhabricatorFileStorageFormat',
+        'mime-type' => 'optional string',
+        'builtin' => 'optional string',
+        'storageEngines' => 'optional list<PhabricatorFileStorageEngine>',
+      ));
+
     $file_name = idx($params, 'name');
     $this->setName($file_name);
 
     $author_phid = idx($params, 'authorPHID');
     $this->setAuthorPHID($author_phid);
 
-    $file_ttl = idx($params, 'ttl');
-    $this->setTtl($file_ttl);
+    $absolute_ttl = idx($params, 'ttl.absolute');
+    $relative_ttl = idx($params, 'ttl.relative');
+    if ($absolute_ttl !== null && $relative_ttl !== null) {
+      throw new Exception(
+        pht(
+          'Specify an absolute TTL or a relative TTL, but not both.'));
+    } else if ($absolute_ttl !== null) {
+      if ($absolute_ttl < PhabricatorTime::getNow()) {
+        throw new Exception(
+          pht(
+            'Absolute TTL must be in the present or future, but TTL "%s" '.
+            'is in the past.',
+            $absolute_ttl));
+      }
+
+      $this->setTtl($absolute_ttl);
+    } else if ($relative_ttl !== null) {
+      if ($relative_ttl < 0) {
+        throw new Exception(
+          pht(
+            'Relative TTL must be zero or more seconds, but "%s" is '.
+            'negative.',
+            $relative_ttl));
+      }
+
+      $max_relative = phutil_units('365 days in seconds');
+      if ($relative_ttl > $max_relative) {
+        throw new Exception(
+          pht(
+            'Relative TTL must not be more than "%s" seconds, but TTL '.
+            '"%s" was specified.',
+            $max_relative,
+            $relative_ttl));
+      }
+
+      $absolute_ttl = PhabricatorTime::getNow() + $relative_ttl;
+
+      $this->setTtl($absolute_ttl);
+    }
 
     $view_policy = idx($params, 'viewPolicy');
     if ($view_policy) {
diff --git a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
index f9090c9c83..5b64c9b1ac 100644
--- a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
+++ b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
@@ -370,11 +370,11 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase {
 
     $data = Filesystem::readRandomCharacters(64);
 
-    $ttl = (time() + 60 * 60 * 24);
+    $ttl = (PhabricatorTime::getNow() + phutil_units('24 hours in seconds'));
 
     $params = array(
       'name' => 'test.dat',
-      'ttl'  => ($ttl),
+      'ttl.absolute' => $ttl,
       'storageEngines' => array(
         $engine,
       ),
diff --git a/src/applications/files/uploadsource/PhabricatorFileUploadSource.php b/src/applications/files/uploadsource/PhabricatorFileUploadSource.php
index 1d38402066..b4578e9e56 100644
--- a/src/applications/files/uploadsource/PhabricatorFileUploadSource.php
+++ b/src/applications/files/uploadsource/PhabricatorFileUploadSource.php
@@ -4,7 +4,7 @@ abstract class PhabricatorFileUploadSource
   extends Phobject {
 
   private $name;
-  private $ttl;
+  private $relativeTTL;
   private $viewPolicy;
 
   private $rope;
@@ -22,13 +22,13 @@ abstract class PhabricatorFileUploadSource
     return $this->name;
   }
 
-  public function setTTL($ttl) {
-    $this->ttl = $ttl;
+  public function setRelativeTTL($relative_ttl) {
+    $this->relativeTTL = $relative_ttl;
     return $this;
   }
 
-  public function getTTL() {
-    return $this->ttl;
+  public function getRelativeTTL() {
+    return $this->relativeTTL;
   }
 
   public function setViewPolicy($view_policy) {
@@ -214,7 +214,7 @@ abstract class PhabricatorFileUploadSource
   private function getNewFileParameters() {
     return array(
       'name' => $this->getName(),
-      'ttl' => $this->getTTL(),
+      'ttl.relative' => $this->getRelativeTTL(),
       'viewPolicy' => $this->getViewPolicy(),
     );
   }
diff --git a/src/applications/phragment/conduit/PhragmentGetPatchConduitAPIMethod.php b/src/applications/phragment/conduit/PhragmentGetPatchConduitAPIMethod.php
index aa9e008007..0a3736f93a 100644
--- a/src/applications/phragment/conduit/PhragmentGetPatchConduitAPIMethod.php
+++ b/src/applications/phragment/conduit/PhragmentGetPatchConduitAPIMethod.php
@@ -178,7 +178,7 @@ final class PhragmentGetPatchConduitAPIMethod
         $data,
         array(
           'name' => 'patch.dmp',
-          'ttl' => time() + 60 * 60 * 24,
+          'ttl.relative' => phutil_units('24 hours in seconds'),
         ));
       $patches[$key]['patchURI'] = $file->getDownloadURI();
     }
diff --git a/src/applications/phragment/controller/PhragmentPatchController.php b/src/applications/phragment/controller/PhragmentPatchController.php
index 5ad4b6dfec..dade7b2f89 100644
--- a/src/applications/phragment/controller/PhragmentPatchController.php
+++ b/src/applications/phragment/controller/PhragmentPatchController.php
@@ -83,7 +83,7 @@ final class PhragmentPatchController extends PhragmentController {
       array(
         'name' => $name,
         'mime-type' => 'text/plain',
-        'ttl' => time() + 60 * 60 * 24,
+        'ttl.relative' => phutil_units('24 hours in seconds'),
       ));
 
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
diff --git a/src/applications/phragment/controller/PhragmentZIPController.php b/src/applications/phragment/controller/PhragmentZIPController.php
index 75d464d9c8..6cb9d3ac77 100644
--- a/src/applications/phragment/controller/PhragmentZIPController.php
+++ b/src/applications/phragment/controller/PhragmentZIPController.php
@@ -104,7 +104,7 @@ final class PhragmentZIPController extends PhragmentController {
       $data,
       array(
         'name' => $zip_name,
-        'ttl' => time() + 60 * 60 * 24,
+        'ttl.relative' => phutil_units('24 hours in seconds'),
       ));
 
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();

From 873b39be8211c118db4d06029cd7b8d80e04f775 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 14:26:02 -0700
Subject: [PATCH 08/28] Remove PhabricatorFile::buildFromFileDataOrHash()

Summary:
Ref T12464. This is a very old method which can return an existing file instead of creating a new one, if there's some existing file with the same content.

In the best case this is a bad idea. This being somewhat reasonable predates policies, temporary files, etc. Modern methods like `newFromFileData()` do this right: they share underlying data in storage, but not the actual `File` records.

Specifically, this is the case where we get into trouble:

  - I upload a private file with content "X".
  - You somehow generate a file with the same content by, say, viewing a raw diff in Differential.
  - If the diff had the same content, you get my file, but you don't have permission to see it or whatever so everything breaks and is terrible.

Just get rid of this.

Test Plan:
  - Generated an SSH key.
  - Viewed a raw diff in Differential.
  - (Did not test Phragment.)

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17617
---
 ...habricatorAuthSSHKeyGenerateController.php |  2 +-
 .../DifferentialRevisionViewController.php    | 16 ++++----
 .../files/storage/PhabricatorFile.php         | 38 -------------------
 .../PhragmentGetPatchConduitAPIMethod.php     | 15 +++++---
 .../controller/PhragmentPatchController.php   | 16 ++++----
 .../controller/PhragmentZIPController.php     | 13 ++++---
 6 files changed, 33 insertions(+), 67 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php
index 070131b443..f39707618d 100644
--- a/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php
+++ b/src/applications/auth/controller/PhabricatorAuthSSHKeyGenerateController.php
@@ -24,7 +24,7 @@ final class PhabricatorAuthSSHKeyGenerateController
       $keys = PhabricatorSSHKeyGenerator::generateKeypair();
       list($public_key, $private_key) = $keys;
 
-      $file = PhabricatorFile::buildFromFileDataOrHash(
+      $file = PhabricatorFile::newFromFileData(
         $private_key,
         array(
           'name' => $default_name.'.key',
diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php
index 26ff22838f..6cb39c1510 100644
--- a/src/applications/differential/controller/DifferentialRevisionViewController.php
+++ b/src/applications/differential/controller/DifferentialRevisionViewController.php
@@ -889,15 +889,15 @@ final class DifferentialRevisionViewController extends DifferentialController {
     }
     $file_name .= 'diff';
 
-    $file = PhabricatorFile::buildFromFileDataOrHash(
-      $raw_diff,
-      array(
-        'name' => $file_name,
-        'ttl.relative' => phutil_units('24 hours in seconds'),
-        'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
-      ));
-
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      $file = PhabricatorFile::newFromFileData(
+        $raw_diff,
+        array(
+          'name' => $file_name,
+          'ttl.relative' => phutil_units('24 hours in seconds'),
+          'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
+        ));
+
       $file->attachToObject($revision->getPHID());
     unset($unguarded);
 
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index 4136f1fdf8..a7dd6b2e97 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -197,44 +197,6 @@ final class PhabricatorFile extends PhabricatorFileDAO
   }
 
 
-  /**
-   * Given a block of data, try to load an existing file with the same content
-   * if one exists. If it does not, build a new file.
-   *
-   * This method is generally used when we have some piece of semi-trusted data
-   * like a diff or a file from a repository that we want to show to the user.
-   * We can't just dump it out because it may be dangerous for any number of
-   * reasons; instead, we need to serve it through the File abstraction so it
-   * ends up on the CDN domain if one is configured and so on. However, if we
-   * simply wrote a new file every time we'd potentially end up with a lot
-   * of redundant data in file storage.
-   *
-   * To solve these problems, we use file storage as a cache and reuse the
-   * same file again if we've previously written it.
-   *
-   * NOTE: This method unguards writes.
-   *
-   * @param string  Raw file data.
-   * @param dict    Dictionary of file information.
-   */
-  public static function buildFromFileDataOrHash(
-    $data,
-    array $params = array()) {
-
-    $file = id(new PhabricatorFile())->loadOneWhere(
-      'name = %s AND contentHash = %s LIMIT 1',
-      idx($params, 'name'),
-      self::hashFileContent($data));
-
-    if (!$file) {
-      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-      $file = self::newFromFileData($data, $params);
-      unset($unguarded);
-    }
-
-    return $file;
-  }
-
   public static function newFileFromContentHash($hash, array $params) {
     // Check to see if a file with same contentHash exist
     $file = id(new PhabricatorFile())->loadOneWhere(
diff --git a/src/applications/phragment/conduit/PhragmentGetPatchConduitAPIMethod.php b/src/applications/phragment/conduit/PhragmentGetPatchConduitAPIMethod.php
index 0a3736f93a..a030190343 100644
--- a/src/applications/phragment/conduit/PhragmentGetPatchConduitAPIMethod.php
+++ b/src/applications/phragment/conduit/PhragmentGetPatchConduitAPIMethod.php
@@ -174,12 +174,15 @@ final class PhragmentGetPatchConduitAPIMethod
       unset($patches[$key]['fileOld']);
       unset($patches[$key]['fileNew']);
 
-      $file = PhabricatorFile::buildFromFileDataOrHash(
-        $data,
-        array(
-          'name' => 'patch.dmp',
-          'ttl.relative' => phutil_units('24 hours in seconds'),
-        ));
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+        $file = PhabricatorFile::newFromFileData(
+          $data,
+          array(
+            'name' => 'patch.dmp',
+            'ttl.relative' => phutil_units('24 hours in seconds'),
+          ));
+      unset($unguarded);
+
       $patches[$key]['patchURI'] = $file->getDownloadURI();
     }
 
diff --git a/src/applications/phragment/controller/PhragmentPatchController.php b/src/applications/phragment/controller/PhragmentPatchController.php
index dade7b2f89..eaa08bc29e 100644
--- a/src/applications/phragment/controller/PhragmentPatchController.php
+++ b/src/applications/phragment/controller/PhragmentPatchController.php
@@ -78,15 +78,15 @@ final class PhragmentPatchController extends PhragmentController {
       $return = $request->getStr('return');
     }
 
-    $result = PhabricatorFile::buildFromFileDataOrHash(
-      $patch,
-      array(
-        'name' => $name,
-        'mime-type' => 'text/plain',
-        'ttl.relative' => phutil_units('24 hours in seconds'),
-      ));
-
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      $result = PhabricatorFile::newFromFileData(
+        $patch,
+        array(
+          'name' => $name,
+          'mime-type' => 'text/plain',
+          'ttl.relative' => phutil_units('24 hours in seconds'),
+        ));
+
       $result->attachToObject($version_b->getFragmentPHID());
     unset($unguarded);
 
diff --git a/src/applications/phragment/controller/PhragmentZIPController.php b/src/applications/phragment/controller/PhragmentZIPController.php
index 6cb9d3ac77..167a67857f 100644
--- a/src/applications/phragment/controller/PhragmentZIPController.php
+++ b/src/applications/phragment/controller/PhragmentZIPController.php
@@ -100,14 +100,15 @@ final class PhragmentZIPController extends PhragmentController {
     }
 
     $data = Filesystem::readFile((string)$temp);
-    $file = PhabricatorFile::buildFromFileDataOrHash(
-      $data,
-      array(
-        'name' => $zip_name,
-        'ttl.relative' => phutil_units('24 hours in seconds'),
-      ));
 
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      $file = PhabricatorFile::newFromFileData(
+        $data,
+        array(
+          'name' => $zip_name,
+          'ttl.relative' => phutil_units('24 hours in seconds'),
+        ));
+
       $file->attachToObject($fragment->getPHID());
     unset($unguarded);
 

From 1e181f0781ca3d603ae0691d90afd2edd47d380b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 14:34:24 -0700
Subject: [PATCH 09/28] Deprecate "file.uploadhash"

Summary:
Ref T12464. This is a very old method which let you create a file on the server by referring to data which already existed in another file.

Basically, long ago, `arc` could say "Do you already have a file with hash X?" and just skip some work if the server did.

`arc` has not called this method since D13017, in May 2015.

Since it's easy to do so, just make this method pretend that it never has the file. Very old clients will continue to work, since they would expect this response in the common case and continue by uploading data.

Test Plan:
  - Grepped for `uploadhash` in Phabricator and Arcanist.
  - Called the method with the console, verified it returned `null`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17618
---
 .../FileUploadHashConduitAPIMethod.php        | 31 ++++++++-----------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/src/applications/files/conduit/FileUploadHashConduitAPIMethod.php b/src/applications/files/conduit/FileUploadHashConduitAPIMethod.php
index 135e018a2f..db7a6acecb 100644
--- a/src/applications/files/conduit/FileUploadHashConduitAPIMethod.php
+++ b/src/applications/files/conduit/FileUploadHashConduitAPIMethod.php
@@ -3,12 +3,21 @@
 final class FileUploadHashConduitAPIMethod extends FileConduitAPIMethod {
 
   public function getAPIMethodName() {
-    // TODO: Deprecate this in favor of `file.allocate`.
     return 'file.uploadhash';
   }
 
+  public function getMethodStatus() {
+    return self::METHOD_STATUS_DEPRECATED;
+  }
+
+  public function getMethodStatusDescription() {
+    return pht(
+      'This method is deprecated. Callers should use "file.allocate" '.
+      'instead.');
+  }
+
   public function getMethodDescription() {
-    return pht('Upload a file to the server using content hash.');
+    return pht('Obsolete. Has no effect.');
   }
 
   protected function defineParamTypes() {
@@ -19,25 +28,11 @@ final class FileUploadHashConduitAPIMethod extends FileConduitAPIMethod {
   }
 
   protected function defineReturnType() {
-    return 'phid or null';
+    return 'null';
   }
 
   protected function execute(ConduitAPIRequest $request) {
-    $hash = $request->getValue('hash');
-    $name = $request->getValue('name');
-    $user = $request->getUser();
-
-    $file = PhabricatorFile::newFileFromContentHash(
-      $hash,
-      array(
-        'name' => $name,
-        'authorPHID' => $user->getPHID(),
-      ));
-
-    if ($file) {
-      return $file->getPHID();
-    }
-    return $file;
+    return null;
   }
 
 }

From 440ef5b7a7e48e2aa2370daa09ffd72f356fedcb Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 14:49:22 -0700
Subject: [PATCH 10/28] Remove SHA1 file content hashing and make Files work
 without any hashing

Summary:
Ref T12464. We currently use SHA1 to detect when two files have the same content so we don't have to store two copies of the data.

Now that a SHA1 collision is known, this is theoretically dangerous. T12464 describes the shape of a possible attack.

Before replacing this with something more robust, shore things up so things work correctly if we don't hash at all. This mechanism is entirely optional; it only helps us store less data if some files are duplicates.

(This mechanism is also less important now than it once was, before we added temporary files.)

Test Plan: Uploaded multiple identical files, saw the uploads work and the files store separate copies of the same data.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17619
---
 .../conduit/FileAllocateConduitAPIMethod.php  | 11 ++-
 .../conduit/FileUploadConduitAPIMethod.php    | 26 ++++---
 .../files/storage/PhabricatorFile.php         | 74 ++++++++++---------
 .../__tests__/PhabricatorFileTestCase.php     |  5 ++
 4 files changed, 68 insertions(+), 48 deletions(-)

diff --git a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php
index ddc5fa14fe..c831eb2afe 100644
--- a/src/applications/files/conduit/FileAllocateConduitAPIMethod.php
+++ b/src/applications/files/conduit/FileAllocateConduitAPIMethod.php
@@ -36,23 +36,26 @@ final class FileAllocateConduitAPIMethod
     $properties = array(
       'name' => $name,
       'authorPHID' => $viewer->getPHID(),
-      'viewPolicy' => $view_policy,
       'isExplicitUpload' => true,
     );
 
+    if ($view_policy !== null) {
+      $properties['viewPolicy'] = $view_policy;
+    }
+
     $ttl = $request->getValue('deleteAfterEpoch');
     if ($ttl) {
       $properties['ttl.absolute'] = $ttl;
     }
 
     $file = null;
-    if ($hash) {
+    if ($hash !== null) {
       $file = PhabricatorFile::newFileFromContentHash(
         $hash,
         $properties);
     }
 
-    if ($hash && !$file) {
+    if ($hash !== null && !$file) {
       $chunked_hash = PhabricatorChunkedFileStorageEngine::getChunkedHash(
         $viewer,
         $hash);
@@ -103,7 +106,7 @@ final class FileAllocateConduitAPIMethod
     if ($chunk_engines) {
       $chunk_properties = $properties;
 
-      if ($hash) {
+      if ($hash !== null) {
         $chunk_properties += array(
           'chunkedHash' => $chunked_hash,
         );
diff --git a/src/applications/files/conduit/FileUploadConduitAPIMethod.php b/src/applications/files/conduit/FileUploadConduitAPIMethod.php
index ac93b7d39a..8b3b9faaaa 100644
--- a/src/applications/files/conduit/FileUploadConduitAPIMethod.php
+++ b/src/applications/files/conduit/FileUploadConduitAPIMethod.php
@@ -27,21 +27,27 @@ final class FileUploadConduitAPIMethod extends FileConduitAPIMethod {
     $viewer = $request->getUser();
 
     $name = $request->getValue('name');
-    $can_cdn = $request->getValue('canCDN');
+    $can_cdn = (bool)$request->getValue('canCDN');
     $view_policy = $request->getValue('viewPolicy');
 
     $data = $request->getValue('data_base64');
     $data = $this->decodeBase64($data);
 
-    $file = PhabricatorFile::newFromFileData(
-      $data,
-      array(
-        'name' => $name,
-        'authorPHID' => $viewer->getPHID(),
-        'viewPolicy' => $view_policy,
-        'canCDN' => $can_cdn,
-        'isExplicitUpload' => true,
-      ));
+    $params = array(
+      'authorPHID' => $viewer->getPHID(),
+      'canCDN' => $can_cdn,
+      'isExplicitUpload' => true,
+    );
+
+    if ($name !== null) {
+      $params['name'] = $name;
+    }
+
+    if ($view_policy !== null) {
+      $params['viewPolicy'] = $view_policy;
+    }
+
+    $file = PhabricatorFile::newFromFileData($data, $params);
 
     return $file->getPHID();
   }
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index a7dd6b2e97..08160f7cbc 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -198,39 +198,42 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
 
   public static function newFileFromContentHash($hash, array $params) {
-    // Check to see if a file with same contentHash exist
+    if ($hash === null) {
+      return null;
+    }
+
+    // Check to see if a file with same hash already exists.
     $file = id(new PhabricatorFile())->loadOneWhere(
       'contentHash = %s LIMIT 1',
       $hash);
-
-    if ($file) {
-      $copy_of_storage_engine = $file->getStorageEngine();
-      $copy_of_storage_handle = $file->getStorageHandle();
-      $copy_of_storage_format = $file->getStorageFormat();
-      $copy_of_storage_properties = $file->getStorageProperties();
-      $copy_of_byte_size = $file->getByteSize();
-      $copy_of_mime_type = $file->getMimeType();
-
-      $new_file = self::initializeNewFile();
-
-      $new_file->setByteSize($copy_of_byte_size);
-
-      $new_file->setContentHash($hash);
-      $new_file->setStorageEngine($copy_of_storage_engine);
-      $new_file->setStorageHandle($copy_of_storage_handle);
-      $new_file->setStorageFormat($copy_of_storage_format);
-      $new_file->setStorageProperties($copy_of_storage_properties);
-      $new_file->setMimeType($copy_of_mime_type);
-      $new_file->copyDimensions($file);
-
-      $new_file->readPropertiesFromParameters($params);
-
-      $new_file->save();
-
-      return $new_file;
+    if (!$file) {
+      return null;
     }
 
-    return $file;
+    $copy_of_storage_engine = $file->getStorageEngine();
+    $copy_of_storage_handle = $file->getStorageHandle();
+    $copy_of_storage_format = $file->getStorageFormat();
+    $copy_of_storage_properties = $file->getStorageProperties();
+    $copy_of_byte_size = $file->getByteSize();
+    $copy_of_mime_type = $file->getMimeType();
+
+    $new_file = self::initializeNewFile();
+
+    $new_file->setByteSize($copy_of_byte_size);
+
+    $new_file->setContentHash($hash);
+    $new_file->setStorageEngine($copy_of_storage_engine);
+    $new_file->setStorageHandle($copy_of_storage_handle);
+    $new_file->setStorageFormat($copy_of_storage_format);
+    $new_file->setStorageProperties($copy_of_storage_properties);
+    $new_file->setMimeType($copy_of_mime_type);
+    $new_file->copyDimensions($file);
+
+    $new_file->readPropertiesFromParameters($params);
+
+    $new_file->save();
+
+    return $new_file;
   }
 
   public static function newChunkedFile(
@@ -353,7 +356,9 @@ final class PhabricatorFile extends PhabricatorFileDAO
     }
 
     $file->setByteSize(strlen($data));
-    $file->setContentHash(self::hashFileContent($data));
+
+    $hash = self::hashFileContent($data);
+    $file->setContentHash($hash);
 
     $file->setStorageEngine($engine_identifier);
     $file->setStorageHandle($data_handle);
@@ -379,10 +384,12 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
   public static function newFromFileData($data, array $params = array()) {
     $hash = self::hashFileContent($data);
-    $file = self::newFileFromContentHash($hash, $params);
 
-    if ($file) {
-      return $file;
+    if ($hash !== null) {
+      $file = self::newFileFromContentHash($hash, $params);
+      if ($file) {
+        return $file;
+      }
     }
 
     return self::buildFromFileData($data, $params);
@@ -710,9 +717,8 @@ final class PhabricatorFile extends PhabricatorFileDAO
     }
   }
 
-
   public static function hashFileContent($data) {
-    return sha1($data);
+    return null;
   }
 
   public function loadFileData() {
diff --git a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
index 5b64c9b1ac..936676b902 100644
--- a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
+++ b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
@@ -301,6 +301,11 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase {
 
     $data = Filesystem::readRandomCharacters(64);
 
+    $hash = PhabricatorFile::hashFileContent($data);
+    if ($hash === null) {
+      $this->assertSkipped(pht('File content hashing is not available.'));
+    }
+
     $params = array(
       'name' => 'test.dat',
       'storageEngines' => array(

From 58011a4e8e7f5f168fd7dd1f53a23a3db1001138 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 15:31:14 -0700
Subject: [PATCH 11/28] Upgrade File content hashing to SHA256

Summary:
Ref T12464. This defuses any possible SHA1-collision attacks by using SHA256, for which there is no known collision.

(SHA256 hashes are larger -- 256 bits -- so expand the storage column to 64 bytes to hold them.)

Test Plan:
  - Uploaded the same file twice, saw the two files generate the same SHA256 content hash and use the same underlying data.
  - Tried with a fake hash algorihtm ("quackxyz") to make sure the failure mode worked/degraded correctly if we don't have SHA256 for some reason. Got two valid files with two copies of the same data, as expected.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12464

Differential Revision: https://secure.phabricator.com/D17620
---
 src/applications/files/storage/PhabricatorFile.php | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index 08160f7cbc..1dfba734a7 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -94,7 +94,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
         'storageHandle' => 'text255',
         'authorPHID' => 'phid?',
         'secretKey' => 'bytes20?',
-        'contentHash' => 'bytes40?',
+        'contentHash' => 'bytes64?',
         'ttl' => 'epoch?',
         'isExplicitUpload' => 'bool?',
         'mailKey' => 'bytes20',
@@ -718,7 +718,17 @@ final class PhabricatorFile extends PhabricatorFileDAO
   }
 
   public static function hashFileContent($data) {
-    return null;
+    // NOTE: Hashing can fail if the algorithm isn't available in the current
+    // build of PHP. It's fine if we're unable to generate a content hash:
+    // it just means we'll store extra data when users upload duplicate files
+    // instead of being able to deduplicate it.
+
+    $hash = hash('sha256', $data, $raw_output = false);
+    if ($hash === false) {
+      return null;
+    }
+
+    return $hash;
   }
 
   public function loadFileData() {

From f70ff34d97e86fe2f0881935de1817977d958535 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 18:33:59 -0700
Subject: [PATCH 12/28] Fix a copy/paste typo with sticky accept

The root issue here is actually just that I cherry-picked stable locally
but did not push it. However, this is a minor issue I also caught while
double-checking things.

Auditors: chad
---
 .../differential/editor/DifferentialTransactionEditor.php       | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index f24bc7abda..dd6666c653 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -344,7 +344,7 @@ final class DifferentialTransactionEditor
       $downgrade[] = DifferentialReviewerStatus::STATUS_ACCEPTED;
     }
 
-    if ($downgrade_accepts) {
+    if ($downgrade_rejects) {
       $downgrade[] = DifferentialReviewerStatus::STATUS_REJECTED;
     }
 

From 45fc4f6f64077e8f239d47c642dc41300e8a0059 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 5 Apr 2017 06:20:02 -0700
Subject: [PATCH 13/28] Iterate over ranges correctly for encryped files

Summary:
Fixes T12079. Currently, when a file is encrypted and a request has "Content-Range", we apply the range first, //then// decrypt the result. This doesn't work since you can't start decrypting something from somewhere in the middle (at least, not with our cipher selection).

Instead: decrypt the result, //then// apply the range.

Test Plan: Added failing unit tests, made them pass

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12079

Differential Revision: https://secure.phabricator.com/D17623
---
 .../PhabricatorChunkedFileStorageEngine.php    | 11 ++++++++++-
 .../engine/PhabricatorFileStorageEngine.php    | 18 ++++++++++++++----
 .../PhabricatorFileStorageFormatTestCase.php   | 18 ++++++++++++++++++
 .../files/storage/PhabricatorFile.php          | 10 +++++++---
 4 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php b/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php
index 8deb1f6465..0bbeeeac6c 100644
--- a/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php
+++ b/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php
@@ -174,7 +174,16 @@ final class PhabricatorChunkedFileStorageEngine
     return (4 * 1024 * 1024);
   }
 
-  public function getRawFileDataIterator(PhabricatorFile $file, $begin, $end) {
+  public function getRawFileDataIterator(
+    PhabricatorFile $file,
+    $begin,
+    $end,
+    PhabricatorFileStorageFormat $format) {
+
+    // NOTE: It is currently impossible for files stored with the chunk
+    // engine to have their own formatting (instead, the individual chunks
+    // are formatted), so we ignore the format object.
+
     $chunks = id(new PhabricatorFileChunkQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
       ->withChunkHandles(array($file->getStorageHandle()))
diff --git a/src/applications/files/engine/PhabricatorFileStorageEngine.php b/src/applications/files/engine/PhabricatorFileStorageEngine.php
index 21f15eab50..39660f847d 100644
--- a/src/applications/files/engine/PhabricatorFileStorageEngine.php
+++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php
@@ -325,10 +325,20 @@ abstract class PhabricatorFileStorageEngine extends Phobject {
     return $engine->getChunkSize();
   }
 
-  public function getRawFileDataIterator(PhabricatorFile $file, $begin, $end) {
-    // The default implementation is trivial and just loads the entire file
-    // upfront.
-    $data = $this->readFile($file->getStorageHandle());
+  public function getRawFileDataIterator(
+    PhabricatorFile $file,
+    $begin,
+    $end,
+    PhabricatorFileStorageFormat $format) {
+
+    $formatted_data = $this->readFile($file->getStorageHandle());
+    $formatted_data = array($formatted_data);
+
+    $data = '';
+    $format_iterator = $format->newReadIterator($formatted_data);
+    foreach ($format_iterator as $raw_chunk) {
+      $data .= $raw_chunk;
+    }
 
     if ($begin !== null && $end !== null) {
       $data = substr($data, $begin, ($end - $begin));
diff --git a/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php b/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php
index 18364fd2a4..77d161045f 100644
--- a/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php
+++ b/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php
@@ -33,6 +33,15 @@ final class PhabricatorFileStorageFormatTestCase extends PhabricatorTestCase {
     // The actual raw data in the storage engine should be encoded.
     $raw_data = $engine->readFile($file->getStorageHandle());
     $this->assertEqual($expect, $raw_data);
+
+    // If we generate an iterator over a slice of the file, it should return
+    // the decrypted file.
+    $iterator = $file->getFileDataIterator(4, 14);
+    $raw_data = '';
+    foreach ($iterator as $data_chunk) {
+      $raw_data .= $data_chunk;
+    }
+    $this->assertEqual('cow jumped', $raw_data);
   }
 
   public function testAES256Storage() {
@@ -73,5 +82,14 @@ final class PhabricatorFileStorageFormatTestCase extends PhabricatorTestCase {
     // input data.
     $raw_data = $engine->readFile($file->getStorageHandle());
     $this->assertTrue($data !== $raw_data);
+
+    // If we generate an iterator over a slice of the file, it should return
+    // the decrypted file.
+    $iterator = $file->getFileDataIterator(4, 14);
+    $raw_data = '';
+    foreach ($iterator as $data_chunk) {
+      $raw_data .= $data_chunk;
+    }
+    $this->assertEqual('cow jumped', $raw_data);
   }
 }
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index 1dfba734a7..da65477202 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -746,14 +746,18 @@ final class PhabricatorFile extends PhabricatorFileDAO
    */
   public function getFileDataIterator($begin = null, $end = null) {
     $engine = $this->instantiateStorageEngine();
-    $raw_iterator = $engine->getRawFileDataIterator($this, $begin, $end);
 
     $key = $this->getStorageFormat();
-
     $format = id(clone PhabricatorFileStorageFormat::requireFormat($key))
       ->setFile($this);
 
-    return $format->newReadIterator($raw_iterator);
+    $iterator = $engine->getRawFileDataIterator(
+      $this,
+      $begin,
+      $end,
+      $format);
+
+    return $iterator;
   }
 
   public function getURI() {

From c4c3de7bfaf34c8d85b5deee6cd25f0ff42f4315 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 5 Apr 2017 06:46:26 -0700
Subject: [PATCH 14/28] Correct an issue where the wrong "Content-Length" was
 set for partial content responses

Summary: Ref T8266. Although we compute this correctly above, we ignored it when actually setting the header. Use the computed value to set the "Content-Length" header. This is consistent with the spec/documentation.

Test Plan: Before, some audio (like `rain.mp3`) was pretty spotty about loading in Safari. It now loads consistently for me locally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8266

Differential Revision: https://secure.phabricator.com/D17624
---
 src/aphront/response/AphrontFileResponse.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/aphront/response/AphrontFileResponse.php b/src/aphront/response/AphrontFileResponse.php
index a8f673d218..6337c984b9 100644
--- a/src/aphront/response/AphrontFileResponse.php
+++ b/src/aphront/response/AphrontFileResponse.php
@@ -105,7 +105,7 @@ final class AphrontFileResponse extends AphrontResponse {
     }
 
     if (!$this->shouldCompressResponse()) {
-      $headers[] = array('Content-Length', $this->getContentLength());
+      $headers[] = array('Content-Length', $content_len);
     }
 
     if (strlen($this->getDownload())) {

From 63828f580689b7799d8d38ed4604e91385f33505 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 5 Apr 2017 09:04:44 -0700
Subject: [PATCH 15/28] Store and verify content integrity checksums for files

Summary:
Ref T12470. This helps defuse attacks where an adversary can directly take control of whatever storage engine files are being stored in and change data there. These attacks would require a significant level of access.

Such attackers could potentially attack ranges of AES-256-CBC encrypted files by using Phabricator as a decryption oracle if they were also able to compromise a Phabricator account with read access to the files.

By storing a hash of the data (and, in the case of AES-256-CBC files, the IV) when we write files, and verifying it before we decrypt or read them, we can detect and prevent this kind of tampering.

This also helps detect mundane corruption and integrity issues.

Test Plan:
  - Added unit tests.
  - Uploaded new files, saw them get integrity hashes.
  - Manually corrupted file data, saw it fail. Used `bin/files cat --salvage` to read it anyway.
  - Tampered with IVs, saw integrity failures.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12470

Differential Revision: https://secure.phabricator.com/D17625
---
 src/__phutil_library_map__.php                |  2 ++
 .../engine/PhabricatorFileStorageEngine.php   | 24 ++++++++++++++
 .../engine/PhabricatorTestStorageEngine.php   |  4 +++
 .../PhabricatorFileIntegrityException.php     |  4 +++
 .../PhabricatorFileAES256StorageFormat.php    | 14 ++++++++
 .../format/PhabricatorFileStorageFormat.php   |  4 +++
 .../PhabricatorFileStorageFormatTestCase.php  | 33 +++++++++++++++++++
 .../PhabricatorFilesManagementCatWorkflow.php | 32 ++++++++++++++++--
 .../files/storage/PhabricatorFile.php         | 24 ++++++++++++--
 9 files changed, 135 insertions(+), 6 deletions(-)
 create mode 100644 src/applications/files/exception/PhabricatorFileIntegrityException.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index f082c2fe22..3fa9b2e25e 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2762,6 +2762,7 @@ phutil_register_library_map(array(
     'PhabricatorFileImageProxyController' => 'applications/files/controller/PhabricatorFileImageProxyController.php',
     'PhabricatorFileImageTransform' => 'applications/files/transform/PhabricatorFileImageTransform.php',
     'PhabricatorFileInfoController' => 'applications/files/controller/PhabricatorFileInfoController.php',
+    'PhabricatorFileIntegrityException' => 'applications/files/exception/PhabricatorFileIntegrityException.php',
     'PhabricatorFileLightboxController' => 'applications/files/controller/PhabricatorFileLightboxController.php',
     'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php',
     'PhabricatorFileListController' => 'applications/files/controller/PhabricatorFileListController.php',
@@ -7891,6 +7892,7 @@ phutil_register_library_map(array(
     'PhabricatorFileImageProxyController' => 'PhabricatorFileController',
     'PhabricatorFileImageTransform' => 'PhabricatorFileTransform',
     'PhabricatorFileInfoController' => 'PhabricatorFileController',
+    'PhabricatorFileIntegrityException' => 'Exception',
     'PhabricatorFileLightboxController' => 'PhabricatorFileController',
     'PhabricatorFileLinkView' => 'AphrontTagView',
     'PhabricatorFileListController' => 'PhabricatorFileController',
diff --git a/src/applications/files/engine/PhabricatorFileStorageEngine.php b/src/applications/files/engine/PhabricatorFileStorageEngine.php
index 39660f847d..9a15460ec8 100644
--- a/src/applications/files/engine/PhabricatorFileStorageEngine.php
+++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php
@@ -332,6 +332,18 @@ abstract class PhabricatorFileStorageEngine extends Phobject {
     PhabricatorFileStorageFormat $format) {
 
     $formatted_data = $this->readFile($file->getStorageHandle());
+
+    $known_integrity = $file->getIntegrityHash();
+    if ($known_integrity !== null) {
+      $new_integrity = $this->newIntegrityHash($formatted_data, $format);
+      if ($known_integrity !== $new_integrity) {
+        throw new PhabricatorFileIntegrityException(
+          pht(
+            'File data integrity check failed. Dark forces have corrupted '.
+            'or tampered with this file. The file data can not be read.'));
+      }
+    }
+
     $formatted_data = array($formatted_data);
 
     $data = '';
@@ -351,4 +363,16 @@ abstract class PhabricatorFileStorageEngine extends Phobject {
     return array($data);
   }
 
+  public function newIntegrityHash(
+    $data,
+    PhabricatorFileStorageFormat $format) {
+
+    $data_hash = PhabricatorHash::digest($data);
+    $format_hash = $format->newFormatIntegrityHash();
+
+    $full_hash = "{$data_hash}/{$format_hash}";
+
+    return PhabricatorHash::digest($full_hash);
+  }
+
 }
diff --git a/src/applications/files/engine/PhabricatorTestStorageEngine.php b/src/applications/files/engine/PhabricatorTestStorageEngine.php
index 72e68dc7c7..952fba6075 100644
--- a/src/applications/files/engine/PhabricatorTestStorageEngine.php
+++ b/src/applications/files/engine/PhabricatorTestStorageEngine.php
@@ -47,4 +47,8 @@ final class PhabricatorTestStorageEngine
     unset(self::$storage[$handle]);
   }
 
+  public function tamperWithFile($handle, $data) {
+    self::$storage[$handle] = $data;
+  }
+
 }
diff --git a/src/applications/files/exception/PhabricatorFileIntegrityException.php b/src/applications/files/exception/PhabricatorFileIntegrityException.php
new file mode 100644
index 0000000000..f1d1989da8
--- /dev/null
+++ b/src/applications/files/exception/PhabricatorFileIntegrityException.php
@@ -0,0 +1,4 @@
+<?php
+
+final class PhabricatorFileIntegrityException
+  extends Exception {}
diff --git a/src/applications/files/format/PhabricatorFileAES256StorageFormat.php b/src/applications/files/format/PhabricatorFileAES256StorageFormat.php
index 6c8a0273bd..79f4595d3e 100644
--- a/src/applications/files/format/PhabricatorFileAES256StorageFormat.php
+++ b/src/applications/files/format/PhabricatorFileAES256StorageFormat.php
@@ -56,6 +56,20 @@ final class PhabricatorFileAES256StorageFormat
     return array($data);
   }
 
+  public function newFormatIntegrityHash() {
+    $file = $this->getFile();
+    list($key_envelope, $iv_envelope) = $this->extractKeyAndIV($file);
+
+    // NOTE: We include the IV in the format integrity hash. If we do not,
+    // attackers can potentially forge the first block of decrypted data
+    // in CBC mode if they are able to substitute a chosen IV and predict
+    // the plaintext. (Normally, they can not tamper with the IV.)
+
+    $input = self::FORMATKEY.'/iv:'.$iv_envelope->openEnvelope();
+
+    return PhabricatorHash::digest($input);
+  }
+
   public function newStorageProperties() {
     // Generate a unique key and IV for this block of data.
     $key_envelope = self::newAES256Key();
diff --git a/src/applications/files/format/PhabricatorFileStorageFormat.php b/src/applications/files/format/PhabricatorFileStorageFormat.php
index 0d286ecd64..2d24aa2207 100644
--- a/src/applications/files/format/PhabricatorFileStorageFormat.php
+++ b/src/applications/files/format/PhabricatorFileStorageFormat.php
@@ -22,6 +22,10 @@ abstract class PhabricatorFileStorageFormat
   abstract public function newReadIterator($raw_iterator);
   abstract public function newWriteIterator($raw_iterator);
 
+  public function newFormatIntegrityHash() {
+    return null;
+  }
+
   public function newStorageProperties() {
     return array();
   }
diff --git a/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php b/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php
index 77d161045f..db10b87964 100644
--- a/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php
+++ b/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php
@@ -92,4 +92,37 @@ final class PhabricatorFileStorageFormatTestCase extends PhabricatorTestCase {
     }
     $this->assertEqual('cow jumped', $raw_data);
   }
+
+  public function testStorageTampering() {
+    $engine = new PhabricatorTestStorageEngine();
+
+    $good = 'The cow jumped over the full moon.';
+    $evil = 'The cow slept quietly, honoring the glorious dictator.';
+
+    $params = array(
+      'name' => 'message.txt',
+      'storageEngines' => array(
+        $engine,
+      ),
+    );
+
+    // First, write the file normally.
+    $file = PhabricatorFile::newFromFileData($good, $params);
+    $this->assertEqual($good, $file->loadFileData());
+
+    // As an adversary, tamper with the file.
+    $engine->tamperWithFile($file->getStorageHandle(), $evil);
+
+    // Attempts to read the file data should now fail the integrity check.
+    $caught = null;
+    try {
+      $file->loadFileData();
+    } catch (PhabricatorFileIntegrityException $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertTrue($caught instanceof PhabricatorFileIntegrityException);
+  }
+
+
 }
diff --git a/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php
index 98efd2dee9..70d5eca5a2 100644
--- a/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php
+++ b/src/applications/files/management/PhabricatorFilesManagementCatWorkflow.php
@@ -19,6 +19,13 @@ final class PhabricatorFilesManagementCatWorkflow
             'param' => 'bytes',
             'help' => pht('End printing at a specific offset.'),
           ),
+          array(
+            'name' => 'salvage',
+            'help' => pht(
+              'DANGEROUS. Attempt to salvage file content even if the '.
+              'integrity check fails. If an adversary has tampered with '.
+              'the file, the conent may be unsafe.'),
+          ),
           array(
             'name'      => 'names',
             'wildcard'  => true,
@@ -43,9 +50,28 @@ final class PhabricatorFilesManagementCatWorkflow
     $begin = $args->getArg('begin');
     $end = $args->getArg('end');
 
-    $iterator = $file->getFileDataIterator($begin, $end);
-    foreach ($iterator as $data) {
-      echo $data;
+    $file->makeEphemeral();
+
+    // If we're running in "salvage" mode, wipe out any integrity hash which
+    // may be present. This makes us read file data without performing an
+    // integrity check.
+    $salvage = $args->getArg('salvage');
+    if ($salvage) {
+      $file->setIntegrityHash(null);
+    }
+
+    try {
+      $iterator = $file->getFileDataIterator($begin, $end);
+      foreach ($iterator as $data) {
+        echo $data;
+      }
+    } catch (PhabricatorFileIntegrityException $ex) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'File data integrity check failed. Use "--salvage" to bypass '.
+          'integrity checks. This flag is dangerous, use it at your own '.
+          'risk. Underlying error: %s',
+          $ex->getMessage()));
     }
 
     return 0;
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index da65477202..dd474d9aa6 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -37,6 +37,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
   const METADATA_PARTIAL = 'partial';
   const METADATA_PROFILE = 'profile';
   const METADATA_STORAGE = 'storage';
+  const METADATA_INTEGRITY = 'integrity';
 
   protected $name;
   protected $mimeType;
@@ -324,15 +325,18 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
     $data_handle = null;
     $engine_identifier = null;
+    $integrity_hash = null;
     $exceptions = array();
     foreach ($engines as $engine) {
       $engine_class = get_class($engine);
       try {
-        list($engine_identifier, $data_handle) = $file->writeToEngine(
+        $result = $file->writeToEngine(
           $engine,
           $data,
           $params);
 
+        list($engine_identifier, $data_handle, $integrity_hash) = $result;
+
         // We stored the file somewhere so stop trying to write it to other
         // places.
         break;
@@ -363,6 +367,8 @@ final class PhabricatorFile extends PhabricatorFileDAO
     $file->setStorageEngine($engine_identifier);
     $file->setStorageHandle($data_handle);
 
+    $file->setIntegrityHash($integrity_hash);
+
     $file->readPropertiesFromParameters($params);
 
     if (!$file->getMimeType()) {
@@ -409,7 +415,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
       'name' => $this->getName(),
     );
 
-    list($new_identifier, $new_handle) = $this->writeToEngine(
+    list($new_identifier, $new_handle, $integrity_hash) = $this->writeToEngine(
       $engine,
       $data,
       $params);
@@ -420,6 +426,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
     $this->setStorageEngine($new_identifier);
     $this->setStorageHandle($new_handle);
+    $this->setIntegrityHash($integrity_hash);
     $this->save();
 
     if (!$make_copy) {
@@ -494,6 +501,8 @@ final class PhabricatorFile extends PhabricatorFileDAO
     $formatted_iterator = $format->newWriteIterator($data_iterator);
     $formatted_data = $this->loadDataFromIterator($formatted_iterator);
 
+    $integrity_hash = $engine->newIntegrityHash($formatted_data, $format);
+
     $data_handle = $engine->writeFile($formatted_data, $params);
 
     if (!$data_handle || strlen($data_handle) > 255) {
@@ -518,7 +527,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
           $engine_identifier));
     }
 
-    return array($engine_identifier, $data_handle);
+    return array($engine_identifier, $data_handle, $integrity_hash);
   }
 
 
@@ -1220,6 +1229,15 @@ final class PhabricatorFile extends PhabricatorFileDAO
     return $this;
   }
 
+  public function setIntegrityHash($integrity_hash) {
+    $this->metadata[self::METADATA_INTEGRITY] = $integrity_hash;
+    return $this;
+  }
+
+  public function getIntegrityHash() {
+    return idx($this->metadata, self::METADATA_INTEGRITY);
+  }
+
   /**
    * Write the policy edge between this file and some object.
    *

From d1a971e22156d5eafa4f3e33231b0f21c6b9e8bd Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 5 Apr 2017 10:51:40 -0700
Subject: [PATCH 16/28] Support "Range: bytes=123-" requests

Summary:
Ref T12219. We currently only support Range requests like "bytes=123-456", but "bytes=123-", meaning "until end of file", is valid, and Chrome can send these requests.

I suspect this is the issue with T12219.

Test Plan: Used `nc local.phacility.com 80` to pipe raw requests, saw both "bytes=123-456" and "bytes=123-" requests satisfied correctly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12219

Differential Revision: https://secure.phabricator.com/D17626
---
 src/aphront/response/AphrontFileResponse.php        |  5 +++++
 .../controller/PhabricatorFileDataController.php    | 13 ++++++++++---
 .../PhabricatorFileStorageFormatTestCase.php        |  8 ++++++++
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/aphront/response/AphrontFileResponse.php b/src/aphront/response/AphrontFileResponse.php
index 6337c984b9..25750ddacd 100644
--- a/src/aphront/response/AphrontFileResponse.php
+++ b/src/aphront/response/AphrontFileResponse.php
@@ -97,7 +97,12 @@ final class AphrontFileResponse extends AphrontResponse {
     if ($this->rangeMin || $this->rangeMax) {
       $len = $this->getContentLength();
       $min = $this->rangeMin;
+
       $max = $this->rangeMax;
+      if ($max === null) {
+        $max = ($len - 1);
+      }
+
       $headers[] = array('Content-Range', "bytes {$min}-{$max}/{$len}");
       $content_len = ($max - $min) + 1;
     } else {
diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php
index 43b5aa419f..31761d1244 100644
--- a/src/applications/files/controller/PhabricatorFileDataController.php
+++ b/src/applications/files/controller/PhabricatorFileDataController.php
@@ -64,14 +64,21 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
     $range = $request->getHTTPHeader('range');
     if ($range) {
       $matches = null;
-      if (preg_match('/^bytes=(\d+)-(\d+)$/', $range, $matches)) {
+      if (preg_match('/^bytes=(\d+)-(\d*)$/', $range, $matches)) {
         // Note that the "Range" header specifies bytes differently than
         // we do internally: the range 0-1 has 2 bytes (byte 0 and byte 1).
         $begin = (int)$matches[1];
-        $end = (int)$matches[2] + 1;
+
+        // The "Range" may be "200-299" or "200-", meaning "until end of file".
+        if (strlen($matches[2])) {
+          $range_end = (int)$matches[2];
+          $end = $range_end + 1;
+        } else {
+          $range_end = null;
+        }
 
         $response->setHTTPResponseCode(206);
-        $response->setRange($begin, ($end - 1));
+        $response->setRange($begin, $range_end);
       }
     }
 
diff --git a/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php b/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php
index db10b87964..b3425ba313 100644
--- a/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php
+++ b/src/applications/files/format/__tests__/PhabricatorFileStorageFormatTestCase.php
@@ -91,6 +91,14 @@ final class PhabricatorFileStorageFormatTestCase extends PhabricatorTestCase {
       $raw_data .= $data_chunk;
     }
     $this->assertEqual('cow jumped', $raw_data);
+
+    $iterator = $file->getFileDataIterator(4, null);
+    $raw_data = '';
+    foreach ($iterator as $data_chunk) {
+      $raw_data .= $data_chunk;
+    }
+    $this->assertEqual('cow jumped over the full moon.', $raw_data);
+
   }
 
   public function testStorageTampering() {

From f2a26b2601000765fcd79f7654f90fc3abdd09e2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 5 Apr 2017 11:31:59 -0700
Subject: [PATCH 17/28] When requesting file data, make "Range: bytes=0-" work
 correctly

Summary: Ref T12219. Chrome can send requests with a "Range: bytes=0-" header, which just means "the whole file", but we don't respond correctly because of a `null` vs `0` issue.

Test Plan: Sent a raw `bytes=0-` request, saw a proper resonse.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12219

Differential Revision: https://secure.phabricator.com/D17627
---
 src/aphront/response/AphrontFileResponse.php | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/aphront/response/AphrontFileResponse.php b/src/aphront/response/AphrontFileResponse.php
index 25750ddacd..9699c49ad4 100644
--- a/src/aphront/response/AphrontFileResponse.php
+++ b/src/aphront/response/AphrontFileResponse.php
@@ -94,7 +94,7 @@ final class AphrontFileResponse extends AphrontResponse {
       array('Accept-Ranges', 'bytes'),
     );
 
-    if ($this->rangeMin || $this->rangeMax) {
+    if ($this->rangeMin !== null || $this->rangeMax !== null) {
       $len = $this->getContentLength();
       $min = $this->rangeMin;
 

From 2f4ff6a850012643ca7e794d3e89a62e51fc1a37 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 5 Apr 2017 12:56:01 -0700
Subject: [PATCH 18/28] Fix bad "editPolicy" key in Paste

Summary: Fixes T12508. Files don't have an `editPolicy`, and we started actually checking that the keys are real things in D17616.

Test Plan:
  - Before patch: created a paste, got an "editPolicy" exception.
  - After patch: created a paste that worked properly.

Reviewers: avivey, chad

Reviewed By: avivey

Maniphest Tasks: T12508

Differential Revision: https://secure.phabricator.com/D17628
---
 .../paste/xaction/PhabricatorPasteContentTransaction.php         | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/applications/paste/xaction/PhabricatorPasteContentTransaction.php b/src/applications/paste/xaction/PhabricatorPasteContentTransaction.php
index 0c3fa7e25b..c90892345d 100644
--- a/src/applications/paste/xaction/PhabricatorPasteContentTransaction.php
+++ b/src/applications/paste/xaction/PhabricatorPasteContentTransaction.php
@@ -79,7 +79,6 @@ final class PhabricatorPasteContentTransaction
         'mime-type' => 'text/plain; charset=utf-8',
         'authorPHID' => $actor->getPHID(),
         'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
-        'editPolicy' => PhabricatorPolicies::POLICY_NOONE,
       ));
   }
 

From cefbdbcffe9e90e0a76c0a1ca3c93668975c1c7a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Apr 2017 14:07:09 -0700
Subject: [PATCH 19/28] Provide a "Reviewers" attachment to
 "differential.revision.search"

Summary: Allow API callers to retrieve reviewer information via a new "reviewers" attachment.

Test Plan: {F4675784}

Reviewers: chad, lvital

Reviewed By: lvital

Subscribers: lvital

Differential Revision: https://secure.phabricator.com/D17633
---
 src/__phutil_library_map__.php                |  2 +
 ...rentialReviewersSearchEngineAttachment.php | 41 +++++++++++++++++++
 .../storage/DifferentialRevision.php          |  5 ++-
 3 files changed, 47 insertions(+), 1 deletion(-)
 create mode 100644 src/applications/differential/engineextension/DifferentialReviewersSearchEngineAttachment.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 3fa9b2e25e..28e808026a 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -502,6 +502,7 @@ phutil_register_library_map(array(
     'DifferentialReviewersCommitMessageField' => 'applications/differential/field/DifferentialReviewersCommitMessageField.php',
     'DifferentialReviewersField' => 'applications/differential/customfield/DifferentialReviewersField.php',
     'DifferentialReviewersHeraldAction' => 'applications/differential/herald/DifferentialReviewersHeraldAction.php',
+    'DifferentialReviewersSearchEngineAttachment' => 'applications/differential/engineextension/DifferentialReviewersSearchEngineAttachment.php',
     'DifferentialReviewersView' => 'applications/differential/view/DifferentialReviewersView.php',
     'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php',
     'DifferentialRevisionAbandonTransaction' => 'applications/differential/xaction/DifferentialRevisionAbandonTransaction.php',
@@ -5275,6 +5276,7 @@ phutil_register_library_map(array(
     'DifferentialReviewersCommitMessageField' => 'DifferentialCommitMessageField',
     'DifferentialReviewersField' => 'DifferentialCoreCustomField',
     'DifferentialReviewersHeraldAction' => 'HeraldAction',
+    'DifferentialReviewersSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment',
     'DifferentialReviewersView' => 'AphrontView',
     'DifferentialRevision' => array(
       'DifferentialDAO',
diff --git a/src/applications/differential/engineextension/DifferentialReviewersSearchEngineAttachment.php b/src/applications/differential/engineextension/DifferentialReviewersSearchEngineAttachment.php
new file mode 100644
index 0000000000..33fb606a60
--- /dev/null
+++ b/src/applications/differential/engineextension/DifferentialReviewersSearchEngineAttachment.php
@@ -0,0 +1,41 @@
+<?php
+
+final class DifferentialReviewersSearchEngineAttachment
+  extends PhabricatorSearchEngineAttachment {
+
+  public function getAttachmentName() {
+    return pht('Differential Reviewers');
+  }
+
+  public function getAttachmentDescription() {
+    return pht('Get the reviewers for each revision.');
+  }
+
+  public function willLoadAttachmentData($query, $spec) {
+    $query->needReviewers(true);
+  }
+
+  public function getAttachmentForObject($object, $data, $spec) {
+    $reviewers = $object->getReviewers();
+
+    $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING;
+
+    $list = array();
+    foreach ($reviewers as $reviewer) {
+      $status = $reviewer->getReviewerStatus();
+      $is_blocking = ($status == $status_blocking);
+
+      $list[] = array(
+        'reviewerPHID' => $reviewer->getReviewerPHID(),
+        'status' => $status,
+        'isBlocking' => $is_blocking,
+        'actorPHID' => $reviewer->getLastActorPHID(),
+      );
+    }
+
+    return array(
+      'reviewers' => $list,
+    );
+  }
+
+}
diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
index 72204256e5..0e968dccf4 100644
--- a/src/applications/differential/storage/DifferentialRevision.php
+++ b/src/applications/differential/storage/DifferentialRevision.php
@@ -907,7 +907,10 @@ final class DifferentialRevision extends DifferentialDAO
   }
 
   public function getConduitSearchAttachments() {
-    return array();
+    return array(
+      id(new DifferentialReviewersSearchEngineAttachment())
+        ->setAttachmentKey('reviewers'),
+    );
   }
 
 

From f1eeaaf59f7ff0e1a17aa35cae2b9631ed414a3f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Apr 2017 14:56:21 -0700
Subject: [PATCH 20/28] Fix scope of "Accept" when you don't check all the
 "Force Accept" boxes

Summary:
Ref T12272. I wrote this correctly, then broke it by adding the simplification which treats "accept the defaults" as "accept everything".

This simplification lets us render "epriestley accepted this revision." instead of "epriestley accepted this revision onbehalf of: long, list, of, every, default, reviewer, they, have, authority, over." so it's a good thing, but make it only affect the reviewers it's supposed to affect.

Test Plan:
  - Did an accept with a force-accept available but unchecked.
  - Before patch: incorrectly accepted all possible reviewers.
  - After patch: accepted only checked reviewers.
  - Also checked the force-accept box, accepted, got a proper force-accept.

Reviewers: chad, lvital

Reviewed By: lvital

Maniphest Tasks: T12272

Differential Revision: https://secure.phabricator.com/D17634
---
 .../DifferentialRevisionReviewTransaction.php     | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
index 019c4c036f..d44de8706c 100644
--- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
@@ -183,8 +183,19 @@ abstract class DifferentialRevisionReviewTransaction
     // In all cases, you affect yourself.
     $map[$viewer->getPHID()] = $status;
 
-    // If the user has submitted a specific list of reviewers to act as (by
-    // unchecking some checkboxes under "Accept"), only affect those reviewers.
+    // If we're applying an "accept the defaults" transaction, and this
+    // transaction type uses checkboxes, replace the value with the list of
+    // defaults.
+    if (!is_array($value)) {
+      list($options, $default) = $this->getActionOptions($viewer, $revision);
+      if ($options) {
+        $value = $default;
+      }
+    }
+
+    // If we have a specific list of reviewers to act on, usually because the
+    // user has submitted a specific list of reviewers to act as by
+    // unchecking some checkboxes under "Accept", only affect those reviewers.
     if (is_array($value)) {
       $map = array_select_keys($map, $value);
     }

From 845a7d871666add7954fac3f9986a55fcb125b79 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Apr 2017 15:17:35 -0700
Subject: [PATCH 21/28] Allow the PullLocal daemon to actually hibernate

Summary:
Ref T12298. The PullLocal daemon has had hibernation code for a little while, but it never actually activated because we don't sleep for more than 15 seconds in any case.

Add a maximum sleep instead and use that to control the longest sleep we'll do for hibernation purposes.

Also, when a repository or repository URI is edited, write a NEEDS_UPDATE event into the message table to make sure the daemons de-hibernate.

Test Plan: Used `bin/phd debug pull`, saw the daemon actually hibernate instead of just sleeping for 15 seconds.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17635
---
 src/applications/diffusion/editor/DiffusionURIEditor.php      | 4 ++++
 .../daemon/PhabricatorRepositoryPullLocalDaemon.php           | 3 ++-
 .../repository/editor/PhabricatorRepositoryEditor.php         | 4 ++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/applications/diffusion/editor/DiffusionURIEditor.php b/src/applications/diffusion/editor/DiffusionURIEditor.php
index 9219935d3a..674efbc158 100644
--- a/src/applications/diffusion/editor/DiffusionURIEditor.php
+++ b/src/applications/diffusion/editor/DiffusionURIEditor.php
@@ -507,6 +507,10 @@ final class DiffusionURIEditor
         ->synchronizeWorkingCopyAfterHostingChange();
     }
 
+    $repository->writeStatusMessage(
+      PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
+      null);
+
     return $xactions;
   }
 
diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php
index 4d4b961765..332d67f7af 100644
--- a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php
+++ b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php
@@ -68,6 +68,7 @@ final class PhabricatorRepositoryPullLocalDaemon
     $retry_after = array();
 
     $min_sleep = 15;
+    $max_sleep = phutil_units('5 minutes in seconds');
     $max_futures = 4;
     $futures = array();
     $queue = array();
@@ -228,7 +229,7 @@ final class PhabricatorRepositoryPullLocalDaemon
         continue;
       }
 
-      $should_hibernate = $this->waitForUpdates($min_sleep, $retry_after);
+      $should_hibernate = $this->waitForUpdates($max_sleep, $retry_after);
       if ($should_hibernate) {
         break;
       }
diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php
index 43c95f6b4a..401fca3668 100644
--- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php
+++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php
@@ -634,6 +634,10 @@ final class PhabricatorRepositoryEditor
         ->synchronizeWorkingCopyAfterCreation();
     }
 
+    $object->writeStatusMessage(
+      PhabricatorRepositoryStatusMessage::TYPE_NEEDS_UPDATE,
+      null);
+
     return $xactions;
   }
 

From 08a42254371fc74b65ac4783b3a70b33fe37f07d Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Apr 2017 05:53:51 -0700
Subject: [PATCH 22/28] Provide "bin/files integrity" for debugging,
 maintaining and backfilling integrity hashes

Summary:
Ref T12470. Provides an "integrity" utility which runs in these modes:

  - Verify: check that hashes match.
  - Compute: backfill missing hashes.
  - Strip: remove hashes. Useful for upgrading across a hash change.
  - Corrupt: intentionally corrupt hashes. Useful for debugging.
  - Overwrite: force hash recomputation.

Users normally shouldn't need to run any of this stuff, but this provides a reasonable toolkit for managing integrity hashes.

I'll recommend existing installs use `bin/files integrity --compute all` in the upgrade guidance to backfill hashes for existing files.

Test Plan:
  - Ran the script in many modes against various files, saw expected operation, including:
  - Verified a file, corrupted it, saw it fail.
  - Verified a file, stripped it, saw it have no hash.
  - Stripped a file, computed it, got a clean verify.
  - Stripped a file, overwrote it, got a clean verify.
  - Corrupted a file, overwrote it, got a clean verify.
  - Overwrote a file, overwrote again, got a no-op.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12470

Differential Revision: https://secure.phabricator.com/D17629
---
 src/__phutil_library_map__.php                |   2 +
 .../engine/PhabricatorFileStorageEngine.php   |   2 +-
 ...icatorFilesManagementIntegrityWorkflow.php | 325 ++++++++++++++++++
 .../files/storage/PhabricatorFile.php         |  33 +-
 .../PhabricatorManagementWorkflow.php         |  36 ++
 5 files changed, 391 insertions(+), 7 deletions(-)
 create mode 100644 src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 28e808026a..89401d56eb 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -2810,6 +2810,7 @@ phutil_register_library_map(array(
     'PhabricatorFilesManagementEncodeWorkflow' => 'applications/files/management/PhabricatorFilesManagementEncodeWorkflow.php',
     'PhabricatorFilesManagementEnginesWorkflow' => 'applications/files/management/PhabricatorFilesManagementEnginesWorkflow.php',
     'PhabricatorFilesManagementGenerateKeyWorkflow' => 'applications/files/management/PhabricatorFilesManagementGenerateKeyWorkflow.php',
+    'PhabricatorFilesManagementIntegrityWorkflow' => 'applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php',
     'PhabricatorFilesManagementMigrateWorkflow' => 'applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php',
     'PhabricatorFilesManagementPurgeWorkflow' => 'applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php',
     'PhabricatorFilesManagementRebuildWorkflow' => 'applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php',
@@ -7941,6 +7942,7 @@ phutil_register_library_map(array(
     'PhabricatorFilesManagementEncodeWorkflow' => 'PhabricatorFilesManagementWorkflow',
     'PhabricatorFilesManagementEnginesWorkflow' => 'PhabricatorFilesManagementWorkflow',
     'PhabricatorFilesManagementGenerateKeyWorkflow' => 'PhabricatorFilesManagementWorkflow',
+    'PhabricatorFilesManagementIntegrityWorkflow' => 'PhabricatorFilesManagementWorkflow',
     'PhabricatorFilesManagementMigrateWorkflow' => 'PhabricatorFilesManagementWorkflow',
     'PhabricatorFilesManagementPurgeWorkflow' => 'PhabricatorFilesManagementWorkflow',
     'PhabricatorFilesManagementRebuildWorkflow' => 'PhabricatorFilesManagementWorkflow',
diff --git a/src/applications/files/engine/PhabricatorFileStorageEngine.php b/src/applications/files/engine/PhabricatorFileStorageEngine.php
index 9a15460ec8..527d39ff9a 100644
--- a/src/applications/files/engine/PhabricatorFileStorageEngine.php
+++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php
@@ -336,7 +336,7 @@ abstract class PhabricatorFileStorageEngine extends Phobject {
     $known_integrity = $file->getIntegrityHash();
     if ($known_integrity !== null) {
       $new_integrity = $this->newIntegrityHash($formatted_data, $format);
-      if ($known_integrity !== $new_integrity) {
+      if (!phutil_hashes_are_identical($known_integrity, $new_integrity)) {
         throw new PhabricatorFileIntegrityException(
           pht(
             'File data integrity check failed. Dark forces have corrupted '.
diff --git a/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php
new file mode 100644
index 0000000000..344df71460
--- /dev/null
+++ b/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php
@@ -0,0 +1,325 @@
+<?php
+
+final class PhabricatorFilesManagementIntegrityWorkflow
+  extends PhabricatorFilesManagementWorkflow {
+
+  protected function didConstruct() {
+    $this
+      ->setName('integrity')
+      ->setSynopsis(pht('Verify or recalculate file integrity hashes.'))
+      ->setArguments(
+        array(
+          array(
+            'name'      => 'all',
+            'help'      => pht('Affect all files.'),
+          ),
+          array(
+            'name' => 'strip',
+            'help' => pht(
+              'DANGEROUS. Strip integrity hashes from files. This makes '.
+              'files vulnerable to corruption or tampering.'),
+          ),
+          array(
+            'name' => 'corrupt',
+            'help' => pht(
+              'Corrupt integrity hashes for given files. This is intended '.
+              'for debugging.'),
+          ),
+          array(
+            'name' => 'compute',
+            'help' => pht(
+              'Compute and update integrity hashes for files which do not '.
+              'yet have them.'),
+          ),
+          array(
+            'name' => 'overwrite',
+            'help' => pht(
+              'DANGEROUS. Recompute and update integrity hashes, overwriting '.
+              'invalid hashes. This may mark corrupt or dangerous files as '.
+              'valid.'),
+          ),
+          array(
+            'name' => 'force',
+            'short' => 'f',
+            'help' => pht(
+              'Execute dangerous operations without prompting for '.
+              'confirmation.'),
+          ),
+          array(
+            'name'      => 'names',
+            'wildcard'  => true,
+          ),
+        ));
+  }
+
+  public function execute(PhutilArgumentParser $args) {
+    $modes = array();
+
+    $is_strip = $args->getArg('strip');
+    if ($is_strip) {
+      $modes[] = 'strip';
+    }
+
+    $is_corrupt = $args->getArg('corrupt');
+    if ($is_corrupt) {
+      $modes[] = 'corrupt';
+    }
+
+    $is_compute = $args->getArg('compute');
+    if ($is_compute) {
+      $modes[] = 'compute';
+    }
+
+    $is_overwrite = $args->getArg('overwrite');
+    if ($is_overwrite) {
+      $modes[] = 'overwrite';
+    }
+
+    $is_verify = !$modes;
+    if ($is_verify) {
+      $modes[] = 'verify';
+    }
+
+    if (count($modes) > 1) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'You have selected multiple operation modes (%s). Choose a '.
+          'single mode to operate in.',
+          implode(', ', $modes)));
+    }
+
+    $is_force = $args->getArg('force');
+    if (!$is_force) {
+      $prompt = null;
+      if ($is_strip) {
+        $prompt = pht(
+          'Stripping integrity hashes is dangerous and makes files '.
+          'vulnerable to corruption or tampering.');
+      }
+
+      if ($is_corrupt) {
+        $prompt = pht(
+          'Corrupting integrity hashes will prevent files from being '.
+          'accessed. This mode is intended only for development and '.
+          'debugging.');
+      }
+
+      if ($is_overwrite) {
+        $prompt = pht(
+          'Overwriting integrity hashes is dangerous and may mark files '.
+          'which have been corrupted or tampered with as safe.');
+      }
+
+      if ($prompt) {
+        $this->logWarn(pht('DANGEROUS'), $prompt);
+
+        if (!phutil_console_confirm(pht('Continue anyway?'))) {
+          throw new PhutilArgumentUsageException(pht('Aborted workflow.'));
+        }
+      }
+    }
+
+    $iterator = $this->buildIterator($args);
+    if (!$iterator) {
+      throw new PhutilArgumentUsageException(
+        pht(
+          'Either specify a list of files to affect, or use "--all" to '.
+          'affect all files.'));
+    }
+
+    $failure_count = 0;
+    $total_count = 0;
+
+    foreach ($iterator as $file) {
+      $total_count++;
+      $display_name = $file->getMonogram();
+
+      $old_hash = $file->getIntegrityHash();
+
+      if ($is_strip) {
+        if ($old_hash === null) {
+          $this->logInfo(
+            pht('SKIPPED'),
+            pht(
+              'File "%s" does not have an integrity hash to strip.',
+              $display_name));
+        } else {
+          $file
+            ->setIntegrityHash(null)
+            ->save();
+
+          $this->logWarn(
+            pht('STRIPPED'),
+            pht(
+              'Stripped integrity hash for "%s".',
+              $display_name));
+        }
+
+        continue;
+      }
+
+      $need_hash = ($is_verify && $old_hash) ||
+                   ($is_compute && ($old_hash === null)) ||
+                   ($is_corrupt) ||
+                   ($is_overwrite);
+      if ($need_hash) {
+        try {
+          $new_hash = $file->newIntegrityHash();
+        } catch (Exception $ex) {
+          $failure_count++;
+
+          $this->logFail(
+            pht('ERROR'),
+            pht(
+              'Unable to compute integrity hash for file "%s": %s',
+              $display_name,
+              $ex->getMessage()));
+
+          continue;
+        }
+      } else {
+        $new_hash = null;
+      }
+
+      // NOTE: When running in "corrupt" mode, we only corrupt the hash if
+      // we're able to compute a valid hash. Some files, like chunked files,
+      // do not support integrity hashing so corrupting them would create an
+      // unusual state.
+
+      if ($is_corrupt) {
+        if ($new_hash === null) {
+          $this->logInfo(
+            pht('IGNORED'),
+            pht(
+              'Storage for file "%s" does not support integrity hashing.',
+              $display_name));
+        } else {
+          $file
+            ->setIntegrityHash('<corrupted>')
+            ->save();
+
+          $this->logWarn(
+            pht('CORRUPTED'),
+            pht(
+              'Corrupted integrity hash for file "%s".',
+              $display_name));
+        }
+
+        continue;
+      }
+
+      if ($is_verify) {
+        if ($old_hash === null) {
+          $this->logInfo(
+            pht('NONE'),
+            pht(
+              'File "%s" has no stored integrity hash.',
+              $display_name));
+        } else if ($new_hash === null) {
+          $failure_count++;
+
+          $this->logWarn(
+            pht('UNEXPECTED'),
+            pht(
+              'Storage for file "%s" does not support integrity hashing, '.
+              'but the file has an integrity hash.',
+              $display_name));
+        } else if (phutil_hashes_are_identical($old_hash, $new_hash)) {
+          $this->logOkay(
+            pht('VALID'),
+            pht(
+              'File "%s" has a valid integrity hash.',
+              $display_name));
+        } else {
+          $failure_count++;
+
+          $this->logFail(
+            pht('MISMATCH'),
+            pht(
+              'File "%s" has an invalid integrity hash!',
+              $display_name));
+        }
+
+        continue;
+      }
+
+      if ($is_compute) {
+        if ($old_hash !== null) {
+          $this->logInfo(
+            pht('SKIP'),
+            pht(
+              'File "%s" already has an integrity hash.',
+              $display_name));
+        } else if ($new_hash === null) {
+          $this->logInfo(
+            pht('IGNORED'),
+            pht(
+              'Storage for file "%s" does not support integrity hashing.',
+              $display_name));
+        } else {
+          $file
+            ->setIntegrityHash($new_hash)
+            ->save();
+
+          $this->logOkay(
+            pht('COMPUTE'),
+            pht(
+              'Computed and stored integrity hash for file "%s".',
+              $display_name));
+        }
+
+        continue;
+      }
+
+      if ($is_overwrite) {
+        $same_hash = ($old_hash !== null) &&
+                     ($new_hash !== null) &&
+                     phutil_hashes_are_identical($old_hash, $new_hash);
+
+        if ($new_hash === null) {
+          $this->logInfo(
+            pht('IGNORED'),
+            pht(
+              'Storage for file "%s" does not support integrity hashing.',
+              $display_name));
+        } else if ($same_hash) {
+          $this->logInfo(
+            pht('UNCHANGED'),
+            pht(
+              'File "%s" already has the correct integrity hash.',
+              $display_name));
+        } else {
+          $file
+            ->setIntegrityHash($new_hash)
+            ->save();
+
+          $this->logOkay(
+            pht('OVERWRITE'),
+            pht(
+              'Overwrote integrity hash for file "%s".',
+              $display_name));
+        }
+
+        continue;
+      }
+    }
+
+    if ($failure_count) {
+      $this->logFail(
+        pht('FAIL'),
+        pht(
+          'Processed %s file(s), encountered %s error(s).',
+          new PhutilNumber($total_count),
+          new PhutilNumber($failure_count)));
+    } else {
+      $this->logOkay(
+        pht('DONE'),
+        pht(
+          'Processed %s file(s) with no errors.',
+          new PhutilNumber($total_count)));
+    }
+
+    return 0;
+  }
+
+}
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
index dd474d9aa6..4faecbd557 100644
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -493,9 +493,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
     $engine_class = get_class($engine);
 
-    $key = $this->getStorageFormat();
-    $format = id(clone PhabricatorFileStorageFormat::requireFormat($key))
-      ->setFile($this);
+    $format = $this->newStorageFormat();
 
     $data_iterator = array($data);
     $formatted_iterator = $format->newWriteIterator($data_iterator);
@@ -756,9 +754,7 @@ final class PhabricatorFile extends PhabricatorFileDAO
   public function getFileDataIterator($begin = null, $end = null) {
     $engine = $this->instantiateStorageEngine();
 
-    $key = $this->getStorageFormat();
-    $format = id(clone PhabricatorFileStorageFormat::requireFormat($key))
-      ->setFile($this);
+    $format = $this->newStorageFormat();
 
     $iterator = $engine->getRawFileDataIterator(
       $this,
@@ -1238,6 +1234,21 @@ final class PhabricatorFile extends PhabricatorFileDAO
     return idx($this->metadata, self::METADATA_INTEGRITY);
   }
 
+  public function newIntegrityHash() {
+    $engine = $this->instantiateStorageEngine();
+
+    if ($engine->isChunkEngine()) {
+      return null;
+    }
+
+    $format = $this->newStorageFormat();
+
+    $storage_handle = $this->getStorageHandle();
+    $data = $engine->readFile($storage_handle);
+
+    return $engine->newIntegrityHash($data, $format);
+  }
+
   /**
    * Write the policy edge between this file and some object.
    *
@@ -1406,6 +1417,16 @@ final class PhabricatorFile extends PhabricatorFileDAO
     return $this->assertAttachedKey($this->transforms, $key);
   }
 
+  public function newStorageFormat() {
+    $key = $this->getStorageFormat();
+    $template = PhabricatorFileStorageFormat::requireFormat($key);
+
+    $format = id(clone $template)
+      ->setFile($this);
+
+    return $format;
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 
diff --git a/src/infrastructure/management/PhabricatorManagementWorkflow.php b/src/infrastructure/management/PhabricatorManagementWorkflow.php
index 0cd8c0230e..ae33f5d6cc 100644
--- a/src/infrastructure/management/PhabricatorManagementWorkflow.php
+++ b/src/infrastructure/management/PhabricatorManagementWorkflow.php
@@ -31,4 +31,40 @@ abstract class PhabricatorManagementWorkflow extends PhutilArgumentWorkflow {
       PhabricatorConsoleContentSource::SOURCECONST);
   }
 
+  protected function logInfo($label, $message) {
+    $this->logRaw(
+      tsprintf(
+        "**<bg:blue> %s </bg>** %s\n",
+        $label,
+        $message));
+  }
+
+  protected function logOkay($label, $message) {
+    $this->logRaw(
+      tsprintf(
+        "**<bg:green> %s </bg>** %s\n",
+        $label,
+        $message));
+  }
+
+  protected function logWarn($label, $message) {
+    $this->logRaw(
+      tsprintf(
+        "**<bg:yellow> %s </bg>** %s\n",
+        $label,
+        $message));
+  }
+
+  protected function logFail($label, $message) {
+    $this->logRaw(
+      tsprintf(
+        "**<bg:red> %s </bg>** %s\n",
+        $label,
+        $message));
+  }
+
+  private function logRaw($message) {
+    fprintf(STDERR, '%s', $message);
+  }
+
 }

From d450a088906eb25c7f9427673ea7bc88555e9254 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Apr 2017 08:35:44 -0700
Subject: [PATCH 23/28] Support HMAC+SHA256 with automatic key generation and
 management

Summary:
Ref T12509. This adds support for HMAC+SHA256 (instead of HMAC+SHA1). Although HMAC+SHA1 is not currently broken in any sense, SHA1 has a well-known collision and it's good to look at moving away from HMAC+SHA1.

The new mechanism also automatically generates and stores HMAC keys.

Currently, HMAC keys largely use a per-install constant defined in `security.hmac-key`. In theory this can be changed, but in practice essentially no install changes it.

We generally (in fact, always, I think?) don't use HMAC digests in a way where it matters that this key is well-known, but it's slightly better if this key is unique per class of use cases. Principally, if use cases have unique HMAC keys they are generally less vulnerable to precomputation attacks where an attacker might generate a large number of HMAC hashes of well-known values and use them in a nefarious way. The actual threat here is probably close to nonexistent, but we can harden against it without much extra effort.

Beyond that, this isn't something users should really have to think about or bother configuring.

Test Plan:
  - Added unit tests.
  - Used `bin/files integrity` to verify, strip, and recompute hashes.
  - Tampered with a generated HMAC key, verified it invalidated hashes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17630
---
 .../autopatches/20170406.hmac.01.keystore.sql |  8 ++
 src/__phutil_library_map__.php                |  4 +
 .../auth/storage/PhabricatorAuthHMACKey.php   | 24 ++++++
 .../PhabricatorSecurityConfigOptions.php      |  3 +-
 .../engine/PhabricatorFileStorageEngine.php   |  8 +-
 .../PhabricatorFileAES256StorageFormat.php    |  4 +-
 src/infrastructure/util/PhabricatorHash.php   | 84 +++++++++++++++++++
 .../__tests__/PhabricatorHMACTestCase.php     | 40 +++++++++
 8 files changed, 171 insertions(+), 4 deletions(-)
 create mode 100644 resources/sql/autopatches/20170406.hmac.01.keystore.sql
 create mode 100644 src/applications/auth/storage/PhabricatorAuthHMACKey.php
 create mode 100644 src/infrastructure/util/__tests__/PhabricatorHMACTestCase.php

diff --git a/resources/sql/autopatches/20170406.hmac.01.keystore.sql b/resources/sql/autopatches/20170406.hmac.01.keystore.sql
new file mode 100644
index 0000000000..f7de1c9efa
--- /dev/null
+++ b/resources/sql/autopatches/20170406.hmac.01.keystore.sql
@@ -0,0 +1,8 @@
+CREATE TABLE {$NAMESPACE}_auth.auth_hmackey (
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  keyName VARCHAR(64) NOT NULL COLLATE {$COLLATE_TEXT},
+  keyValue VARCHAR(128) NOT NULL COLLATE {$COLLATE_TEXT},
+  dateCreated INT UNSIGNED NOT NULL,
+  dateModified INT UNSIGNED NOT NULL,
+  UNIQUE KEY `key_name` (keyName)
+) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 89401d56eb..b567e5a9e9 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1920,6 +1920,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthFactorConfig' => 'applications/auth/storage/PhabricatorAuthFactorConfig.php',
     'PhabricatorAuthFactorTestCase' => 'applications/auth/factor/__tests__/PhabricatorAuthFactorTestCase.php',
     'PhabricatorAuthFinishController' => 'applications/auth/controller/PhabricatorAuthFinishController.php',
+    'PhabricatorAuthHMACKey' => 'applications/auth/storage/PhabricatorAuthHMACKey.php',
     'PhabricatorAuthHighSecurityRequiredException' => 'applications/auth/exception/PhabricatorAuthHighSecurityRequiredException.php',
     'PhabricatorAuthHighSecurityToken' => 'applications/auth/data/PhabricatorAuthHighSecurityToken.php',
     'PhabricatorAuthInvite' => 'applications/auth/storage/PhabricatorAuthInvite.php',
@@ -2864,6 +2865,7 @@ phutil_register_library_map(array(
     'PhabricatorGuideModule' => 'applications/guides/module/PhabricatorGuideModule.php',
     'PhabricatorGuideModuleController' => 'applications/guides/controller/PhabricatorGuideModuleController.php',
     'PhabricatorGuideQuickStartModule' => 'applications/guides/module/PhabricatorGuideQuickStartModule.php',
+    'PhabricatorHMACTestCase' => 'infrastructure/util/__tests__/PhabricatorHMACTestCase.php',
     'PhabricatorHTTPParameterTypeTableView' => 'applications/config/view/PhabricatorHTTPParameterTypeTableView.php',
     'PhabricatorHandleList' => 'applications/phid/handle/pool/PhabricatorHandleList.php',
     'PhabricatorHandleObjectSelectorDataView' => 'applications/phid/handle/view/PhabricatorHandleObjectSelectorDataView.php',
@@ -6900,6 +6902,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthFactorConfig' => 'PhabricatorAuthDAO',
     'PhabricatorAuthFactorTestCase' => 'PhabricatorTestCase',
     'PhabricatorAuthFinishController' => 'PhabricatorAuthController',
+    'PhabricatorAuthHMACKey' => 'PhabricatorAuthDAO',
     'PhabricatorAuthHighSecurityRequiredException' => 'Exception',
     'PhabricatorAuthHighSecurityToken' => 'Phobject',
     'PhabricatorAuthInvite' => array(
@@ -7998,6 +8001,7 @@ phutil_register_library_map(array(
     'PhabricatorGuideModule' => 'Phobject',
     'PhabricatorGuideModuleController' => 'PhabricatorGuideController',
     'PhabricatorGuideQuickStartModule' => 'PhabricatorGuideModule',
+    'PhabricatorHMACTestCase' => 'PhabricatorTestCase',
     'PhabricatorHTTPParameterTypeTableView' => 'AphrontView',
     'PhabricatorHandleList' => array(
       'Phobject',
diff --git a/src/applications/auth/storage/PhabricatorAuthHMACKey.php b/src/applications/auth/storage/PhabricatorAuthHMACKey.php
new file mode 100644
index 0000000000..4d2aae4997
--- /dev/null
+++ b/src/applications/auth/storage/PhabricatorAuthHMACKey.php
@@ -0,0 +1,24 @@
+<?php
+
+final class PhabricatorAuthHMACKey
+  extends PhabricatorAuthDAO {
+
+  protected $keyName;
+  protected $keyValue;
+
+  protected function getConfiguration() {
+    return array(
+      self::CONFIG_COLUMN_SCHEMA => array(
+        'keyName' => 'text64',
+        'keyValue' => 'text128',
+      ),
+      self::CONFIG_KEY_SCHEMA => array(
+        'key_name' => array(
+          'columns' => array('keyName'),
+          'unique' => true,
+        ),
+      ),
+    ) + parent::getConfiguration();
+  }
+
+}
diff --git a/src/applications/config/option/PhabricatorSecurityConfigOptions.php b/src/applications/config/option/PhabricatorSecurityConfigOptions.php
index 47fd1bb6f2..78f8e89e6a 100644
--- a/src/applications/config/option/PhabricatorSecurityConfigOptions.php
+++ b/src/applications/config/option/PhabricatorSecurityConfigOptions.php
@@ -109,7 +109,8 @@ EOTEXT
             'Default key for HMAC digests where the key is not important '.
             '(i.e., the hash itself is secret). You can change this if you '.
             'want (to any other string), but doing so will break existing '.
-            'sessions and CSRF tokens.')),
+            'sessions and CSRF tokens. This option is deprecated. Newer '.
+            'code automatically manages HMAC keys.')),
       $this->newOption('security.require-https', 'bool', false)
         ->setLocked(true)
         ->setSummary(
diff --git a/src/applications/files/engine/PhabricatorFileStorageEngine.php b/src/applications/files/engine/PhabricatorFileStorageEngine.php
index 527d39ff9a..4f2847f8e0 100644
--- a/src/applications/files/engine/PhabricatorFileStorageEngine.php
+++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php
@@ -16,6 +16,8 @@
  */
 abstract class PhabricatorFileStorageEngine extends Phobject {
 
+  const HMAC_INTEGRITY = 'file.integrity';
+
   /**
    * Construct a new storage engine.
    *
@@ -367,12 +369,14 @@ abstract class PhabricatorFileStorageEngine extends Phobject {
     $data,
     PhabricatorFileStorageFormat $format) {
 
-    $data_hash = PhabricatorHash::digest($data);
+    $hmac_name = self::HMAC_INTEGRITY;
+
+    $data_hash = PhabricatorHash::digestWithNamedKey($data, $hmac_name);
     $format_hash = $format->newFormatIntegrityHash();
 
     $full_hash = "{$data_hash}/{$format_hash}";
 
-    return PhabricatorHash::digest($full_hash);
+    return PhabricatorHash::digestWithNamedKey($full_hash, $hmac_name);
   }
 
 }
diff --git a/src/applications/files/format/PhabricatorFileAES256StorageFormat.php b/src/applications/files/format/PhabricatorFileAES256StorageFormat.php
index 79f4595d3e..1990a2509d 100644
--- a/src/applications/files/format/PhabricatorFileAES256StorageFormat.php
+++ b/src/applications/files/format/PhabricatorFileAES256StorageFormat.php
@@ -67,7 +67,9 @@ final class PhabricatorFileAES256StorageFormat
 
     $input = self::FORMATKEY.'/iv:'.$iv_envelope->openEnvelope();
 
-    return PhabricatorHash::digest($input);
+    return PhabricatorHash::digestWithNamedKey(
+      $input,
+      PhabricatorFileStorageEngine::HMAC_INTEGRITY);
   }
 
   public function newStorageProperties() {
diff --git a/src/infrastructure/util/PhabricatorHash.php b/src/infrastructure/util/PhabricatorHash.php
index 05b5fa719d..dc13b85dbe 100644
--- a/src/infrastructure/util/PhabricatorHash.php
+++ b/src/infrastructure/util/PhabricatorHash.php
@@ -139,5 +139,89 @@ final class PhabricatorHash extends Phobject {
     return $prefix.'-'.$hash;
   }
 
+  public static function digestWithNamedKey($message, $key_name) {
+    $key_bytes = self::getNamedHMACKey($key_name);
+    return self::digestHMACSHA256($message, $key_bytes);
+  }
+
+  public static function digestHMACSHA256($message, $key) {
+    if (!strlen($key)) {
+      throw new Exception(
+        pht('HMAC-SHA256 requires a nonempty key.'));
+    }
+
+    $result = hash_hmac('sha256', $message, $key, $raw_output = false);
+
+    if ($result === false) {
+      throw new Exception(
+        pht('Unable to compute HMAC-SHA256 digest of message.'));
+    }
+
+    return $result;
+  }
+
+
+/* -(  HMAC Key Management  )------------------------------------------------ */
+
+
+  private static function getNamedHMACKey($hmac_name) {
+    $cache = PhabricatorCaches::getImmutableCache();
+
+    $cache_key = "hmac.key({$hmac_name})";
+
+    $hmac_key = $cache->getKey($cache_key);
+    if (!strlen($hmac_key)) {
+      $hmac_key = self::readHMACKey($hmac_name);
+
+      if ($hmac_key === null) {
+        $hmac_key = self::newHMACKey($hmac_name);
+        self::writeHMACKey($hmac_name, $hmac_key);
+      }
+
+      $cache->setKey($cache_key, $hmac_key);
+    }
+
+    // The "hex2bin()" function doesn't exist until PHP 5.4.0 so just
+    // implement it inline.
+    $result = '';
+    for ($ii = 0; $ii < strlen($hmac_key); $ii += 2) {
+      $result .= pack('H*', substr($hmac_key, $ii, 2));
+    }
+
+    return $result;
+  }
+
+  private static function newHMACKey($hmac_name) {
+    $hmac_key = Filesystem::readRandomBytes(64);
+    return bin2hex($hmac_key);
+  }
+
+  private static function writeHMACKey($hmac_name, $hmac_key) {
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+
+      id(new PhabricatorAuthHMACKey())
+        ->setKeyName($hmac_name)
+        ->setKeyValue($hmac_key)
+        ->save();
+
+    unset($unguarded);
+  }
+
+  private static function readHMACKey($hmac_name) {
+    $table = new PhabricatorAuthHMACKey();
+    $conn = $table->establishConnection('r');
+
+    $row = queryfx_one(
+      $conn,
+      'SELECT keyValue FROM %T WHERE keyName = %s',
+      $table->getTableName(),
+      $hmac_name);
+    if (!$row) {
+      return null;
+    }
+
+    return $row['keyValue'];
+  }
+
 
 }
diff --git a/src/infrastructure/util/__tests__/PhabricatorHMACTestCase.php b/src/infrastructure/util/__tests__/PhabricatorHMACTestCase.php
new file mode 100644
index 0000000000..90fe1bf37b
--- /dev/null
+++ b/src/infrastructure/util/__tests__/PhabricatorHMACTestCase.php
@@ -0,0 +1,40 @@
+<?php
+
+final class PhabricatorHMACTestCase extends PhabricatorTestCase {
+
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array(
+      self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true,
+    );
+  }
+
+  public function testHMACKeyGeneration() {
+    $input = 'quack';
+
+    $hash_1 = PhabricatorHash::digestWithNamedKey($input, 'test');
+    $hash_2 = PhabricatorHash::digestWithNamedKey($input, 'test');
+
+    $this->assertEqual($hash_1, $hash_2);
+  }
+
+  public function testSHA256Hashing() {
+    $input = 'quack';
+    $key = 'duck';
+    $expect =
+      '5274473dc34fc61bd7a6a5ff258e6505'.
+      '4b26644fb7a272d74f276ab677361b9a';
+
+    $hash = PhabricatorHash::digestHMACSHA256($input, $key);
+    $this->assertEqual($expect, $hash);
+
+    $input = 'The quick brown fox jumps over the lazy dog';
+    $key = 'key';
+    $expect =
+      'f7bc83f430538424b13298e6aa6fb143'.
+      'ef4d59a14946175997479dbc2d1a3cd8';
+
+    $hash = PhabricatorHash::digestHMACSHA256($input, $key);
+    $this->assertEqual($expect, $hash);
+  }
+
+}

From 3a3626834e60578a22a3356c0c97b6cd1b766f6b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Apr 2017 09:55:37 -0700
Subject: [PATCH 24/28] Replace Remarkup calls to `PhabricatorHash::digest()`
 with SHA256

Summary:
Ref T12509. Many of the calls to HMAC+SHA1 are just to compute cachekeys for remarkup objects.

Make these use HMAC+SHA256 instead. There is no downside to swapping these since they just cause a cache miss in the worst case.

I also plan to get rid of `PhabricatorMarkupInterface` eventually, but this doesn't go that far.

Test Plan: Browsed some different types of documents (tasks, legalpad documents, phame blogs / posts, pholio mocks, etc).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17631
---
 .../calendar/storage/PhabricatorCalendarEvent.php |  5 ++---
 .../storage/DifferentialInlineComment.php         |  4 ++--
 .../legalpad/storage/LegalpadDocumentBody.php     |  4 ++--
 .../maniphest/storage/ManiphestTask.php           |  5 ++---
 src/applications/phame/storage/PhameBlog.php      |  4 ++--
 src/applications/phame/storage/PhamePost.php      |  4 ++--
 src/applications/pholio/storage/PholioImage.php   |  4 ++--
 src/applications/pholio/storage/PholioMock.php    |  4 ++--
 .../phriction/storage/PhrictionContent.php        |  8 ++------
 src/applications/ponder/storage/PonderAnswer.php  |  5 ++---
 .../ponder/storage/PonderQuestion.php             |  5 ++---
 .../specification/ReleephFieldSpecification.php   |  6 ++++--
 .../markup/PhabricatorMarkupEngine.php            | 15 +++++++++++++++
 13 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/src/applications/calendar/storage/PhabricatorCalendarEvent.php b/src/applications/calendar/storage/PhabricatorCalendarEvent.php
index b2e81b0e8b..e566a77b76 100644
--- a/src/applications/calendar/storage/PhabricatorCalendarEvent.php
+++ b/src/applications/calendar/storage/PhabricatorCalendarEvent.php
@@ -1182,9 +1182,8 @@ final class PhabricatorCalendarEvent extends PhabricatorCalendarDAO
    * @task markup
    */
   public function getMarkupFieldKey($field) {
-    $hash = PhabricatorHash::digest($this->getMarkupText($field));
-    $id = $this->getID();
-    return "calendar:T{$id}:{$field}:{$hash}";
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
 
diff --git a/src/applications/differential/storage/DifferentialInlineComment.php b/src/applications/differential/storage/DifferentialInlineComment.php
index c27d59bbe3..bdc231671f 100644
--- a/src/applications/differential/storage/DifferentialInlineComment.php
+++ b/src/applications/differential/storage/DifferentialInlineComment.php
@@ -260,8 +260,8 @@ final class DifferentialInlineComment
 
 
   public function getMarkupFieldKey($field) {
-    // We can't use ID because synthetic comments don't have it.
-    return 'DI:'.PhabricatorHash::digest($this->getContent());
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
   public function newMarkupEngine($field) {
diff --git a/src/applications/legalpad/storage/LegalpadDocumentBody.php b/src/applications/legalpad/storage/LegalpadDocumentBody.php
index a3fdf20f5f..001e38ebf3 100644
--- a/src/applications/legalpad/storage/LegalpadDocumentBody.php
+++ b/src/applications/legalpad/storage/LegalpadDocumentBody.php
@@ -39,8 +39,8 @@ final class LegalpadDocumentBody extends LegalpadDAO
 
 
   public function getMarkupFieldKey($field) {
-    $hash = PhabricatorHash::digest($this->getMarkupText($field));
-    return 'LEGB:'.$hash;
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
   public function newMarkupEngine($field) {
diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php
index ebcdef691d..16937da2f2 100644
--- a/src/applications/maniphest/storage/ManiphestTask.php
+++ b/src/applications/maniphest/storage/ManiphestTask.php
@@ -301,9 +301,8 @@ final class ManiphestTask extends ManiphestDAO
    * @task markup
    */
   public function getMarkupFieldKey($field) {
-    $hash = PhabricatorHash::digest($this->getMarkupText($field));
-    $id = $this->getID();
-    return "maniphest:T{$id}:{$field}:{$hash}";
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
 
diff --git a/src/applications/phame/storage/PhameBlog.php b/src/applications/phame/storage/PhameBlog.php
index 32cb6db543..bd4954d0ab 100644
--- a/src/applications/phame/storage/PhameBlog.php
+++ b/src/applications/phame/storage/PhameBlog.php
@@ -289,8 +289,8 @@ final class PhameBlog extends PhameDAO
 
 
   public function getMarkupFieldKey($field) {
-    $hash = PhabricatorHash::digest($this->getMarkupText($field));
-    return $this->getPHID().':'.$field.':'.$hash;
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
 
diff --git a/src/applications/phame/storage/PhamePost.php b/src/applications/phame/storage/PhamePost.php
index 6e9e005bfa..f87a37e7a4 100644
--- a/src/applications/phame/storage/PhamePost.php
+++ b/src/applications/phame/storage/PhamePost.php
@@ -241,8 +241,8 @@ final class PhamePost extends PhameDAO
 
 
   public function getMarkupFieldKey($field) {
-    $hash = PhabricatorHash::digest($this->getMarkupText($field));
-    return $this->getPHID().':'.$field.':'.$hash;
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
   public function newMarkupEngine($field) {
diff --git a/src/applications/pholio/storage/PholioImage.php b/src/applications/pholio/storage/PholioImage.php
index e81c71a05a..70f6e8b8c4 100644
--- a/src/applications/pholio/storage/PholioImage.php
+++ b/src/applications/pholio/storage/PholioImage.php
@@ -84,8 +84,8 @@ final class PholioImage extends PholioDAO
 
 
   public function getMarkupFieldKey($field) {
-    $hash = PhabricatorHash::digest($this->getMarkupText($field));
-    return 'M:'.$hash;
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
   public function newMarkupEngine($field) {
diff --git a/src/applications/pholio/storage/PholioMock.php b/src/applications/pholio/storage/PholioMock.php
index 00e8efd981..7f58a95511 100644
--- a/src/applications/pholio/storage/PholioMock.php
+++ b/src/applications/pholio/storage/PholioMock.php
@@ -217,8 +217,8 @@ final class PholioMock extends PholioDAO
 
 
   public function getMarkupFieldKey($field) {
-    $hash = PhabricatorHash::digest($this->getMarkupText($field));
-    return 'M:'.$hash;
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
   public function newMarkupEngine($field) {
diff --git a/src/applications/phriction/storage/PhrictionContent.php b/src/applications/phriction/storage/PhrictionContent.php
index 3a2e20aa29..0492534797 100644
--- a/src/applications/phriction/storage/PhrictionContent.php
+++ b/src/applications/phriction/storage/PhrictionContent.php
@@ -68,12 +68,8 @@ final class PhrictionContent extends PhrictionDAO
    * @task markup
    */
   public function getMarkupFieldKey($field) {
-    if ($this->shouldUseMarkupCache($field)) {
-      $id = $this->getID();
-    } else {
-      $id = PhabricatorHash::digest($this->getMarkupText($field));
-    }
-    return "phriction:{$field}:{$id}";
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
 
diff --git a/src/applications/ponder/storage/PonderAnswer.php b/src/applications/ponder/storage/PonderAnswer.php
index 76c9057497..f9e3e8eb8d 100644
--- a/src/applications/ponder/storage/PonderAnswer.php
+++ b/src/applications/ponder/storage/PonderAnswer.php
@@ -136,9 +136,8 @@ final class PonderAnswer extends PonderDAO
   // Markup interface
 
   public function getMarkupFieldKey($field) {
-    $hash = PhabricatorHash::digest($this->getMarkupText($field));
-    $id = $this->getID();
-    return "ponder:A{$id}:{$field}:{$hash}";
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
   public function getMarkupText($field) {
diff --git a/src/applications/ponder/storage/PonderQuestion.php b/src/applications/ponder/storage/PonderQuestion.php
index d302bbfc29..6594219f5a 100644
--- a/src/applications/ponder/storage/PonderQuestion.php
+++ b/src/applications/ponder/storage/PonderQuestion.php
@@ -155,9 +155,8 @@ final class PonderQuestion extends PonderDAO
   // Markup interface
 
   public function getMarkupFieldKey($field) {
-    $hash = PhabricatorHash::digest($this->getMarkupText($field));
-    $id = $this->getID();
-    return "ponder:Q{$id}:{$field}:{$hash}";
+    $content = $this->getMarkupText($field);
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
   public function getMarkupText($field) {
diff --git a/src/applications/releeph/field/specification/ReleephFieldSpecification.php b/src/applications/releeph/field/specification/ReleephFieldSpecification.php
index df458ced56..ff95ed6514 100644
--- a/src/applications/releeph/field/specification/ReleephFieldSpecification.php
+++ b/src/applications/releeph/field/specification/ReleephFieldSpecification.php
@@ -236,12 +236,14 @@ abstract class ReleephFieldSpecification
   }
 
   final public function getMarkupFieldKey($field) {
-    return sprintf(
+    $content = sprintf(
       '%s:%s:%s:%s',
       $this->getReleephRequest()->getPHID(),
       $this->getStorageKey(),
       $field,
-      PhabricatorHash::digest($this->getMarkupText($field)));
+      $this->getMarkupText($field));
+
+    return PhabricatorMarkupEngine::digestRemarkupContent($this, $content);
   }
 
   final public function newMarkupEngine($field) {
diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php
index 3fbf29c294..ad380414f6 100644
--- a/src/infrastructure/markup/PhabricatorMarkupEngine.php
+++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php
@@ -694,4 +694,19 @@ final class PhabricatorMarkupEngine extends Phobject {
       ->execute();
   }
 
+  public static function digestRemarkupContent($object, $content) {
+    $parts = array();
+    $parts[] = get_class($object);
+
+    if ($object instanceof PhabricatorLiskDAO) {
+      $parts[] = $object->getID();
+    }
+
+    $parts[] = $content;
+
+    $message = implode("\n", $parts);
+
+    return PhabricatorHash::digestWithNamedKey($message, 'remarkup');
+  }
+
 }

From 3d816e94dfbedd5b9bdea0d9d83674a97bef0651 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Apr 2017 10:08:46 -0700
Subject: [PATCH 25/28] Rename "PhabricatorHash::digest()" to "weakDigest()"

Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.

Test Plan: `grep`, browsed around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12509

Differential Revision: https://secure.phabricator.com/D17632
---
 resources/sql/patches/20130530.sessionhash.php           | 2 +-
 .../auth/controller/PhabricatorAuthController.php        | 2 +-
 .../auth/controller/PhabricatorAuthLoginController.php   | 2 +-
 .../controller/PhabricatorAuthOneTimeLoginController.php | 2 +-
 .../PhabricatorAuthTerminateSessionController.php        | 2 +-
 .../auth/engine/PhabricatorAuthSessionEngine.php         | 8 ++++----
 .../auth/factor/PhabricatorTOTPAuthFactor.php            | 4 ++--
 .../auth/provider/PhabricatorAuthProvider.php            | 2 +-
 .../auth/query/PhabricatorAuthSessionQuery.php           | 2 +-
 .../base/controller/PhabricatorController.php            | 2 +-
 .../celerity/resources/CelerityResources.php             | 2 +-
 .../config/check/PhabricatorPathSetupCheck.php           | 2 +-
 .../diffusion/controller/DiffusionServeController.php    | 2 +-
 .../gitlfs/DiffusionGitLFSTemporaryTokenType.php         | 2 +-
 .../files/engine/PhabricatorChunkedFileStorageEngine.php | 2 +-
 .../metamta/receiver/PhabricatorObjectMailReceiver.php   | 2 +-
 src/applications/people/storage/PhabricatorUser.php      | 6 +++---
 .../settings/panel/PhabricatorPasswordSettingsPanel.php  | 2 +-
 .../settings/panel/PhabricatorSessionsSettingsPanel.php  | 2 +-
 src/infrastructure/util/PhabricatorHash.php              | 9 ++++++---
 20 files changed, 31 insertions(+), 28 deletions(-)

diff --git a/resources/sql/patches/20130530.sessionhash.php b/resources/sql/patches/20130530.sessionhash.php
index 4efbe5feec..771dac61e3 100644
--- a/resources/sql/patches/20130530.sessionhash.php
+++ b/resources/sql/patches/20130530.sessionhash.php
@@ -14,7 +14,7 @@ foreach ($sessions as $session) {
     $conn,
     'UPDATE %T SET sessionKey = %s WHERE userPHID = %s AND type = %s',
     PhabricatorUser::SESSION_TABLE,
-    PhabricatorHash::digest($session['sessionKey']),
+    PhabricatorHash::weakDigest($session['sessionKey']),
     $session['userPHID'],
     $session['type']);
 }
diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php
index 4a572edf22..c5b3c1ce99 100644
--- a/src/applications/auth/controller/PhabricatorAuthController.php
+++ b/src/applications/auth/controller/PhabricatorAuthController.php
@@ -194,7 +194,7 @@ abstract class PhabricatorAuthController extends PhabricatorController {
     // hijacking registration sessions.
 
     $actual = $account->getProperty('registrationKey');
-    $expect = PhabricatorHash::digest($registration_key);
+    $expect = PhabricatorHash::weakDigest($registration_key);
     if (!phutil_hashes_are_identical($actual, $expect)) {
       $response = $this->renderError(
         pht(
diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php
index b9b2a8d876..3264e61216 100644
--- a/src/applications/auth/controller/PhabricatorAuthLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php
@@ -194,7 +194,7 @@ final class PhabricatorAuthLoginController
     $registration_key = Filesystem::readRandomCharacters(32);
     $account->setProperty(
       'registrationKey',
-      PhabricatorHash::digest($registration_key));
+      PhabricatorHash::weakDigest($registration_key));
 
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
       $account->save();
diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
index 627d1b43eb..ebfd07e7ac 100644
--- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php
@@ -135,7 +135,7 @@ final class PhabricatorAuthOneTimeLoginController
             ->setTokenResource($target_user->getPHID())
             ->setTokenType($password_type)
             ->setTokenExpires(time() + phutil_units('1 hour in seconds'))
-            ->setTokenCode(PhabricatorHash::digest($key))
+            ->setTokenCode(PhabricatorHash::weakDigest($key))
             ->save();
         unset($unguarded);
 
diff --git a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php
index 43ac80bb70..fa58977c90 100644
--- a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php
+++ b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php
@@ -16,7 +16,7 @@ final class PhabricatorAuthTerminateSessionController
       $query->withIDs(array($id));
     }
 
-    $current_key = PhabricatorHash::digest(
+    $current_key = PhabricatorHash::weakDigest(
       $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
 
     $sessions = $query->execute();
diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
index 4e66d3c9f4..82303fff2b 100644
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -110,7 +110,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     $session_table = new PhabricatorAuthSession();
     $user_table = new PhabricatorUser();
     $conn_r = $session_table->establishConnection('r');
-    $session_key = PhabricatorHash::digest($session_token);
+    $session_key = PhabricatorHash::weakDigest($session_token);
 
     $cache_parts = $this->getUserCacheQueryParts($conn_r);
     list($cache_selects, $cache_joins, $cache_map, $types_map) = $cache_parts;
@@ -240,7 +240,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     // This has a side effect of validating the session type.
     $session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
 
-    $digest_key = PhabricatorHash::digest($session_key);
+    $digest_key = PhabricatorHash::weakDigest($session_key);
 
     // Logging-in users don't have CSRF stuff yet, so we have to unguard this
     // write.
@@ -306,7 +306,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       ->execute();
 
     if ($except_session !== null) {
-      $except_session = PhabricatorHash::digest($except_session);
+      $except_session = PhabricatorHash::weakDigest($except_session);
     }
 
     foreach ($sessions as $key => $session) {
@@ -755,7 +755,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       $parts[] = $email->getVerificationCode();
     }
 
-    return PhabricatorHash::digest(implode(':', $parts));
+    return PhabricatorHash::weakDigest(implode(':', $parts));
   }
 
 
diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
index 8875b960e2..10c44aaec0 100644
--- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
+++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
@@ -39,7 +39,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
         ->withTokenResources(array($user->getPHID()))
         ->withTokenTypes(array($totp_token_type))
         ->withExpired(false)
-        ->withTokenCodes(array(PhabricatorHash::digest($key)))
+        ->withTokenCodes(array(PhabricatorHash::weakDigest($key)))
         ->executeOne();
       if (!$temporary_token) {
         // If we don't have a matching token, regenerate the key below.
@@ -58,7 +58,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
           ->setTokenResource($user->getPHID())
           ->setTokenType($totp_token_type)
           ->setTokenExpires(time() + phutil_units('1 hour in seconds'))
-          ->setTokenCode(PhabricatorHash::digest($key))
+          ->setTokenCode(PhabricatorHash::weakDigest($key))
           ->save();
       unset($unguarded);
     }
diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php
index a3f77618e4..4dd7f4da1b 100644
--- a/src/applications/auth/provider/PhabricatorAuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorAuthProvider.php
@@ -474,7 +474,7 @@ abstract class PhabricatorAuthProvider extends Phobject {
         true);
     }
 
-    return PhabricatorHash::digest($phcid);
+    return PhabricatorHash::weakDigest($phcid);
   }
 
   protected function verifyAuthCSRFCode(AphrontRequest $request, $actual) {
diff --git a/src/applications/auth/query/PhabricatorAuthSessionQuery.php b/src/applications/auth/query/PhabricatorAuthSessionQuery.php
index 966bb946c4..dea95dd450 100644
--- a/src/applications/auth/query/PhabricatorAuthSessionQuery.php
+++ b/src/applications/auth/query/PhabricatorAuthSessionQuery.php
@@ -85,7 +85,7 @@ final class PhabricatorAuthSessionQuery
     if ($this->sessionKeys) {
       $hashes = array();
       foreach ($this->sessionKeys as $session_key) {
-        $hashes[] = PhabricatorHash::digest($session_key);
+        $hashes[] = PhabricatorHash::weakDigest($session_key);
       }
       $where[] = qsprintf(
         $conn_r,
diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php
index f2b267b67f..58d8f6cf6f 100644
--- a/src/applications/base/controller/PhabricatorController.php
+++ b/src/applications/base/controller/PhabricatorController.php
@@ -98,7 +98,7 @@ abstract class PhabricatorController extends AphrontController {
 
 
       if (!$user->isLoggedIn()) {
-        $user->attachAlternateCSRFString(PhabricatorHash::digest($phsid));
+        $user->attachAlternateCSRFString(PhabricatorHash::weakDigest($phsid));
       }
 
       $request->setUser($user);
diff --git a/src/applications/celerity/resources/CelerityResources.php b/src/applications/celerity/resources/CelerityResources.php
index 9536786a6a..e0b9191702 100644
--- a/src/applications/celerity/resources/CelerityResources.php
+++ b/src/applications/celerity/resources/CelerityResources.php
@@ -14,7 +14,7 @@ abstract class CelerityResources extends Phobject {
 
   public function getCelerityHash($data) {
     $tail = PhabricatorEnv::getEnvConfig('celerity.resource-hash');
-    $hash = PhabricatorHash::digest($data, $tail);
+    $hash = PhabricatorHash::weakDigest($data, $tail);
     return substr($hash, 0, 8);
   }
 
diff --git a/src/applications/config/check/PhabricatorPathSetupCheck.php b/src/applications/config/check/PhabricatorPathSetupCheck.php
index 9f5502e215..75c89d332a 100644
--- a/src/applications/config/check/PhabricatorPathSetupCheck.php
+++ b/src/applications/config/check/PhabricatorPathSetupCheck.php
@@ -107,7 +107,7 @@ final class PhabricatorPathSetupCheck extends PhabricatorSetupCheck {
 
     if ($bad_paths) {
       foreach ($bad_paths as $path_part => $message) {
-        $digest = substr(PhabricatorHash::digest($path_part), 0, 8);
+        $digest = substr(PhabricatorHash::weakDigest($path_part), 0, 8);
 
         $this
           ->newIssue('config.PATH.'.$digest)
diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php
index 02ff23b15f..5e61ae3a7c 100644
--- a/src/applications/diffusion/controller/DiffusionServeController.php
+++ b/src/applications/diffusion/controller/DiffusionServeController.php
@@ -652,7 +652,7 @@ final class DiffusionServeController extends DiffusionController {
     }
 
     $lfs_pass = $password->openEnvelope();
-    $lfs_hash = PhabricatorHash::digest($lfs_pass);
+    $lfs_hash = PhabricatorHash::weakDigest($lfs_pass);
 
     $token = id(new PhabricatorAuthTemporaryTokenQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
diff --git a/src/applications/diffusion/gitlfs/DiffusionGitLFSTemporaryTokenType.php b/src/applications/diffusion/gitlfs/DiffusionGitLFSTemporaryTokenType.php
index 0973072ba4..e5425b07e1 100644
--- a/src/applications/diffusion/gitlfs/DiffusionGitLFSTemporaryTokenType.php
+++ b/src/applications/diffusion/gitlfs/DiffusionGitLFSTemporaryTokenType.php
@@ -22,7 +22,7 @@ final class DiffusionGitLFSTemporaryTokenType
 
     $lfs_user = self::HTTP_USERNAME;
     $lfs_pass = Filesystem::readRandomCharacters(32);
-    $lfs_hash = PhabricatorHash::digest($lfs_pass);
+    $lfs_hash = PhabricatorHash::weakDigest($lfs_pass);
 
     $ttl = PhabricatorTime::getNow() + phutil_units('1 day in seconds');
 
diff --git a/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php b/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php
index 0bbeeeac6c..1bb86cf67e 100644
--- a/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php
+++ b/src/applications/files/engine/PhabricatorChunkedFileStorageEngine.php
@@ -102,7 +102,7 @@ final class PhabricatorChunkedFileStorageEngine
   }
 
   public static function getChunkedHashForInput($input) {
-    $rehash = PhabricatorHash::digest($input);
+    $rehash = PhabricatorHash::weakDigest($input);
 
     // Add a suffix to identify this as a chunk hash.
     $rehash = substr($rehash, 0, -2).'-C';
diff --git a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php
index 52a9a78a19..5985458ab4 100644
--- a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php
+++ b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php
@@ -201,7 +201,7 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
   public static function computeMailHash($mail_key, $phid) {
     $global_mail_key = PhabricatorEnv::getEnvConfig('phabricator.mail-key');
 
-    $hash = PhabricatorHash::digest($mail_key.$global_mail_key.$phid);
+    $hash = PhabricatorHash::weakDigest($mail_key.$global_mail_key.$phid);
     return substr($hash, 0, 16);
   }
 
diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index 7edbcd4e52..4a35d9956c 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -389,7 +389,7 @@ final class PhabricatorUser
     // Generate a token hash to mitigate BREACH attacks against SSL. See
     // discussion in T3684.
     $token = $this->getRawCSRFToken();
-    $hash = PhabricatorHash::digest($token, $salt);
+    $hash = PhabricatorHash::weakDigest($token, $salt);
     return self::CSRF_BREACH_PREFIX.$salt.substr(
         $hash, 0, self::CSRF_TOKEN_LENGTH);
   }
@@ -435,7 +435,7 @@ final class PhabricatorUser
     for ($ii = -$csrf_window; $ii <= 1; $ii++) {
       $valid = $this->getRawCSRFToken($ii);
 
-      $digest = PhabricatorHash::digest($valid, $salt);
+      $digest = PhabricatorHash::weakDigest($valid, $salt);
       $digest = substr($digest, 0, self::CSRF_TOKEN_LENGTH);
       if (phutil_hashes_are_identical($digest, $token)) {
         return true;
@@ -459,7 +459,7 @@ final class PhabricatorUser
     $time_block = floor($epoch / $frequency);
     $vec = $vec.$key.$time_block;
 
-    return substr(PhabricatorHash::digest($vec), 0, $len);
+    return substr(PhabricatorHash::weakDigest($vec), 0, $len);
   }
 
   public function getUserProfile() {
diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
index fa91dac210..17b8cdde95 100644
--- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
@@ -48,7 +48,7 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
         ->setViewer($user)
         ->withTokenResources(array($user->getPHID()))
         ->withTokenTypes(array($password_type))
-        ->withTokenCodes(array(PhabricatorHash::digest($key)))
+        ->withTokenCodes(array(PhabricatorHash::weakDigest($key)))
         ->withExpired(false)
         ->executeOne();
     }
diff --git a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php
index e3a37c9dfc..a468ed53d0 100644
--- a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php
@@ -44,7 +44,7 @@ final class PhabricatorSessionsSettingsPanel extends PhabricatorSettingsPanel {
       ->withPHIDs($identity_phids)
       ->execute();
 
-    $current_key = PhabricatorHash::digest(
+    $current_key = PhabricatorHash::weakDigest(
       $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
 
     $rows = array();
diff --git a/src/infrastructure/util/PhabricatorHash.php b/src/infrastructure/util/PhabricatorHash.php
index dc13b85dbe..19c8414539 100644
--- a/src/infrastructure/util/PhabricatorHash.php
+++ b/src/infrastructure/util/PhabricatorHash.php
@@ -5,12 +5,15 @@ final class PhabricatorHash extends Phobject {
   const INDEX_DIGEST_LENGTH = 12;
 
   /**
-   * Digest a string for general use, including use which relates to security.
+   * Digest a string using HMAC+SHA1.
+   *
+   * Because a SHA1 collision is now known, this method should be considered
+   * weak. Callers should prefer @{method:digestWithNamedKey}.
    *
    * @param   string  Input string.
    * @return  string  32-byte hexidecimal SHA1+HMAC hash.
    */
-  public static function digest($string, $key = null) {
+  public static function weakDigest($string, $key = null) {
     if ($key === null) {
       $key = PhabricatorEnv::getEnvConfig('security.hmac-key');
     }
@@ -37,7 +40,7 @@ final class PhabricatorHash extends Phobject {
     }
 
     for ($ii = 0; $ii < 1000; $ii++) {
-      $result = self::digest($result, $salt);
+      $result = self::weakDigest($result, $salt);
     }
 
     return $result;

From 9856802ba22e86eded8fac38d7a05e809a8d557a Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 6 Apr 2017 15:51:17 -0700
Subject: [PATCH 26/28] Disallow /source/ in robots.txt

Summary: Ref T4245. We disallow `/diffusion/` in robots.txt already because indexers tend to get lost blaming every line of every file throughout history, but didn't update the list for the `/source/` alias. Update it.

Test Plan: Visited `/robots.txt` locally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D17637
---
 .../system/controller/PhabricatorRobotsController.php            | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/applications/system/controller/PhabricatorRobotsController.php b/src/applications/system/controller/PhabricatorRobotsController.php
index e413984f5a..18be8862a8 100644
--- a/src/applications/system/controller/PhabricatorRobotsController.php
+++ b/src/applications/system/controller/PhabricatorRobotsController.php
@@ -20,6 +20,7 @@ final class PhabricatorRobotsController extends PhabricatorController {
 
     $out[] = 'User-Agent: *';
     $out[] = 'Disallow: /diffusion/';
+    $out[] = 'Disallow: /source/';
 
     // Add a small crawl delay (number of seconds between requests) for spiders
     // which respect it. The intent here is to prevent spiders from affecting

From 0bf106eeeaa822b199872351673e38b6f5197ee0 Mon Sep 17 00:00:00 2001
From: Rabah Meradi <rabahmeradi@gmail.com>
Date: Fri, 7 Apr 2017 04:09:56 -0700
Subject: [PATCH 27/28] Fix 4 typos in code

Summary: Fixes T12516

Test Plan: grep for those typos

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12516

Differential Revision: https://secure.phabricator.com/D17638
---
 .../drydock/management/DrydockManagementLeaseWorkflow.php     | 2 +-
 .../stepgroup/HarbormasterExternalBuildStepGroup.php          | 2 +-
 .../people/controller/PhabricatorPeopleLdapController.php     | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php
index baffd7706f..4992833c50 100644
--- a/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php
+++ b/src/applications/drydock/management/DrydockManagementLeaseWorkflow.php
@@ -22,7 +22,7 @@ final class DrydockManagementLeaseWorkflow
           array(
             'name'      => 'attributes',
             'param'     => 'name=value,...',
-            'help'      => pht('Resource specficiation.'),
+            'help'      => pht('Resource specification.'),
           ),
         ));
   }
diff --git a/src/applications/harbormaster/stepgroup/HarbormasterExternalBuildStepGroup.php b/src/applications/harbormaster/stepgroup/HarbormasterExternalBuildStepGroup.php
index 274a25bfbf..df92a2fd90 100644
--- a/src/applications/harbormaster/stepgroup/HarbormasterExternalBuildStepGroup.php
+++ b/src/applications/harbormaster/stepgroup/HarbormasterExternalBuildStepGroup.php
@@ -6,7 +6,7 @@ final class HarbormasterExternalBuildStepGroup
   const GROUPKEY = 'harbormaster.external';
 
   public function getGroupName() {
-    return pht('Interacting with External Build Sytems');
+    return pht('Interacting with External Build Systems');
   }
 
   public function getGroupOrder() {
diff --git a/src/applications/people/controller/PhabricatorPeopleLdapController.php b/src/applications/people/controller/PhabricatorPeopleLdapController.php
index 1a6530ab31..876bf986ad 100644
--- a/src/applications/people/controller/PhabricatorPeopleLdapController.php
+++ b/src/applications/people/controller/PhabricatorPeopleLdapController.php
@@ -38,7 +38,7 @@ final class PhabricatorPeopleLdapController
 
     $crumbs = $this->buildApplicationCrumbs();
     $crumbs->addTextCrumb(
-      pht('Import Ldap Users'),
+      pht('Import LDAP Users'),
       $this->getApplicationURI('/ldap/'));
 
     $nav = $this->buildSideNavView();
@@ -56,7 +56,7 @@ final class PhabricatorPeopleLdapController
     }
 
     return $this->newPage()
-      ->setTitle(pht('Import Ldap Users'))
+      ->setTitle(pht('Import LDAP Users'))
       ->setCrumbs($crumbs)
       ->setNavigation($nav);
   }

From 7707685733d26bf1c7278a2f338416a038c2709b Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 7 Apr 2017 05:33:30 -0700
Subject: [PATCH 28/28] Fix two strings with missing pht()

Summary: Fixes T12517.

Test Plan: Viewed Config application; viewed repository list.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12517

Differential Revision: https://secure.phabricator.com/D17639
---
 .../config/application/PhabricatorConfigApplication.php         | 2 +-
 .../diffusion/controller/DiffusionRepositoryListController.php  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/applications/config/application/PhabricatorConfigApplication.php b/src/applications/config/application/PhabricatorConfigApplication.php
index 510cb6f76d..9789b7de9f 100644
--- a/src/applications/config/application/PhabricatorConfigApplication.php
+++ b/src/applications/config/application/PhabricatorConfigApplication.php
@@ -27,7 +27,7 @@ final class PhabricatorConfigApplication extends PhabricatorApplication {
   }
 
   public function getName() {
-    return 'Config';
+    return pht('Config');
   }
 
   public function getShortDescription() {
diff --git a/src/applications/diffusion/controller/DiffusionRepositoryListController.php b/src/applications/diffusion/controller/DiffusionRepositoryListController.php
index d886209c89..5a21d2e3f1 100644
--- a/src/applications/diffusion/controller/DiffusionRepositoryListController.php
+++ b/src/applications/diffusion/controller/DiffusionRepositoryListController.php
@@ -14,7 +14,7 @@ final class DiffusionRepositoryListController extends DiffusionController {
       ->setName(pht('Commits'));
 
     $items[] = id(new PHUIListItemView())
-      ->setName('Browse Commits')
+      ->setName(pht('Browse Commits'))
       ->setHref($this->getApplicationURI('commit/'));
 
     return id(new PhabricatorRepositorySearchEngine())