Page MenuHomePhabricator
Files F14008612

D15641.id37701.diff
No OneTemporary
Actions

D15641.id37701.diff
View Options

diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/src/applications/files/storage/PhabricatorFile.php
@@ -139,6 +139,10 @@
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
--- a/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
+++ b/src/applications/files/storage/__tests__/PhabricatorFileTestCase.php
@@ -8,6 +8,133 @@
);
}
+ 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
--- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -678,6 +678,10 @@
$editor->save();
break;
+ case PhabricatorTransactions::TYPE_VIEW_POLICY:
+ case PhabricatorTransactions::TYPE_SPACE:
+ $this->scrambleFileSecrets($object);
+ break;
}
}
@@ -914,7 +918,6 @@
}
$xactions = $this->applyFinalEffects($object, $xactions);
-
if ($read_locking) {
$object->endReadLocking();
$read_locking = false;
@@ -3471,4 +3474,64 @@
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;
+ }
+
}

File Metadata

Mime Type
text/plain
Expires
Thu, Oct 31, 2:14 AM (2 w, 5 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6729303
Default Alt Text
D15641.id37701.diff (7 KB)

Event Timeline

Phabricator · Built by Phacility