From f9836cb646f8bbc8f88927bba23a830aebf308ec Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 6 Apr 2016 12:10:33 -0700 Subject: [PATCH] Scramble file secrets when related objects change policies Summary: Ref T10262. Files have an internal secret key which is partially used to control access to them, and determines part of the URL you need to access them. Scramble (regenerate) the secret when: - the view policy for the file itself changes (and the new policy is not "public" or "all users"); or - the view policy or space for an object the file is attached to changes (and the file policy is not "public" or "all users"). This basically means that when you change the visibility of a task, any old URLs for attached files stop working and new ones are implicitly generated. Test Plan: - Attached a file to a task, used `SELECT * FROM file WHERE id = ...` to inspect the secret. - Set view policy to public, same secret. - Set view policy to me, new secret. - Changed task view policy, new secret. - Changed task space, new secret. - Changed task title, same old secret. - Added and ran unit tests which cover this behavior. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10262 Differential Revision: https://secure.phabricator.com/D15641 --- .../files/storage/PhabricatorFile.php | 4 + .../__tests__/PhabricatorFileTestCase.php | 127 ++++++++++++++++++ ...habricatorApplicationTransactionEditor.php | 65 ++++++++- 3 files changed, 195 insertions(+), 1 deletion(-) diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index f1d4c11bd7..81b02f21c5 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -139,6 +139,10 @@ final class PhabricatorFile extends PhabricatorFileDAO return 'F'.$this->getID(); } + public function scrambleSecret() { + return $this->setSecretKey($this->generateSecretKey()); + } + public static function readUploadedFileData($spec) { if (!$spec) { throw new Exception(pht('No file was uploaded!')); diff --git a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php index f6199c9a03..8aa22c6578 100644 --- a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php +++ b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php @@ -8,6 +8,133 @@ final class PhabricatorFileTestCase extends PhabricatorTestCase { ); } + public function testFileDirectScramble() { + // Changes to a file's view policy should scramble the file secret. + + $engine = new PhabricatorTestStorageEngine(); + $data = Filesystem::readRandomCharacters(64); + + $author = $this->generateNewTestUser(); + + $params = array( + 'name' => 'test.dat', + 'viewPolicy' => PhabricatorPolicies::POLICY_USER, + 'authorPHID' => $author->getPHID(), + 'storageEngines' => array( + $engine, + ), + ); + + $file = PhabricatorFile::newFromFileData($data, $params); + + $secret1 = $file->getSecretKey(); + + // First, change the name: this should not scramble the secret. + $xactions = array(); + $xactions[] = id(new PhabricatorFileTransaction()) + ->setTransactionType(PhabricatorFileTransaction::TYPE_NAME) + ->setNewValue('test.dat2'); + + $engine = id(new PhabricatorFileEditor()) + ->setActor($author) + ->setContentSource($this->newContentSource()) + ->applyTransactions($file, $xactions); + + $file = $file->reload(); + + $secret2 = $file->getSecretKey(); + + $this->assertEqual( + $secret1, + $secret2, + pht('No secret scramble on non-policy edit.')); + + // Now, change the view policy. This should scramble the secret. + $xactions = array(); + $xactions[] = id(new PhabricatorFileTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY) + ->setNewValue($author->getPHID()); + + $engine = id(new PhabricatorFileEditor()) + ->setActor($author) + ->setContentSource($this->newContentSource()) + ->applyTransactions($file, $xactions); + + $file = $file->reload(); + $secret3 = $file->getSecretKey(); + + $this->assertTrue( + ($secret1 !== $secret3), + pht('Changing file view policy should scramble secret.')); + } + + public function testFileIndirectScramble() { + // When a file is attached to an object like a task and the task view + // policy changes, the file secret should be scrambled. This invalidates + // old URIs if tasks get locked down. + + $engine = new PhabricatorTestStorageEngine(); + $data = Filesystem::readRandomCharacters(64); + + $author = $this->generateNewTestUser(); + + $params = array( + 'name' => 'test.dat', + 'viewPolicy' => $author->getPHID(), + 'authorPHID' => $author->getPHID(), + 'storageEngines' => array( + $engine, + ), + ); + + $file = PhabricatorFile::newFromFileData($data, $params); + $secret1 = $file->getSecretKey(); + + $task = ManiphestTask::initializeNewTask($author); + + $xactions = array(); + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_TITLE) + ->setNewValue(pht('File Scramble Test Task')); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_DESCRIPTION) + ->setNewValue('{'.$file->getMonogram().'}'); + + id(new ManiphestTransactionEditor()) + ->setActor($author) + ->setContentSource($this->newContentSource()) + ->applyTransactions($task, $xactions); + + $file = $file->reload(); + $secret2 = $file->getSecretKey(); + + $this->assertEqual( + $secret1, + $secret2, + pht( + 'File policy should not scramble when attached to '. + 'newly created object.')); + + $xactions = array(); + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_VIEW_POLICY) + ->setNewValue($author->getPHID()); + + id(new ManiphestTransactionEditor()) + ->setActor($author) + ->setContentSource($this->newContentSource()) + ->applyTransactions($task, $xactions); + + $file = $file->reload(); + $secret3 = $file->getSecretKey(); + + $this->assertTrue( + ($secret1 !== $secret3), + pht('Changing attached object view policy should scramble secret.')); + } + + public function testFileVisibility() { $engine = new PhabricatorTestStorageEngine(); $data = Filesystem::readRandomCharacters(64); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index d7c0bbf5f6..3f51ca7296 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -678,6 +678,10 @@ abstract class PhabricatorApplicationTransactionEditor $editor->save(); break; + case PhabricatorTransactions::TYPE_VIEW_POLICY: + case PhabricatorTransactions::TYPE_SPACE: + $this->scrambleFileSecrets($object); + break; } } @@ -914,7 +918,6 @@ abstract class PhabricatorApplicationTransactionEditor } $xactions = $this->applyFinalEffects($object, $xactions); - if ($read_locking) { $object->endReadLocking(); $read_locking = false; @@ -3471,4 +3474,64 @@ abstract class PhabricatorApplicationTransactionEditor return $phids; } + /** + * When the view policy for an object is changed, scramble the secret keys + * for attached files to invalidate existing URIs. + */ + private function scrambleFileSecrets($object) { + // If this is a newly created object, we don't need to scramble anything + // since it couldn't have been previously published. + if ($this->getIsNewObject()) { + return; + } + + // If the object is a file itself, scramble it. + if ($object instanceof PhabricatorFile) { + if ($this->shouldScramblePolicy($object->getViewPolicy())) { + $object->scrambleSecret(); + $object->save(); + } + } + + $phid = $object->getPHID(); + + $attached_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $phid, + PhabricatorObjectHasFileEdgeType::EDGECONST); + if (!$attached_phids) { + return; + } + + $omnipotent_viewer = PhabricatorUser::getOmnipotentUser(); + + $files = id(new PhabricatorFileQuery()) + ->setViewer($omnipotent_viewer) + ->withPHIDs($attached_phids) + ->execute(); + foreach ($files as $file) { + $view_policy = $file->getViewPolicy(); + if ($this->shouldScramblePolicy($view_policy)) { + $file->scrambleSecret(); + $file->save(); + } + } + } + + + /** + * Check if a policy is strong enough to justify scrambling. Objects which + * are set to very open policies don't need to scramble their files, and + * files with very open policies don't need to be scrambled when associated + * objects change. + */ + private function shouldScramblePolicy($policy) { + switch ($policy) { + case PhabricatorPolicies::POLICY_PUBLIC: + case PhabricatorPolicies::POLICY_USER: + return false; + } + + return true; + } + }