From 873b39be8211c118db4d06029cd7b8d80e04f775 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 4 Apr 2017 14:26:02 -0700 Subject: [PATCH] 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);