mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-15 01:50:55 +01:00
(stable) Correct an issue where "View Raw File" in Differential generated a file with overbroad permissions
Summary: Via HackerOne. When you view a raw file in Differential, we currently generate a permanent file with default permissions. This may be incorrect: default permissions may be broader than the diff's permissions. The other three methods of downloading/viewing raw files ("Download" in Diffusion and Differential, "View Raw" in Diffusion and Differential) already apply policies correctly and generate temporary files. However, this workflow was missed when other workflows were updated. Beyond updating the workflow, delete any files we've generated in the past. This wipes the slate clean on any security issues and frees up a little disk space. Test Plan: - Ran migration script, saw existing files get purged. - Did "View Raw File", got a new file. - Verified that the file was temporary and properly attached to the diff, with "NO ONE" permissions. - Double-checked that Diffusion already runs policy logic correctly and applies appropriate policies. - Double-checked that "Download Raw Diff" in Differential already runs policy logic correctly. - Double-chekced that "Download Raw Diff" in Diffusion already runs policy logic correctly. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D17504
This commit is contained in:
parent
ae5f797c4d
commit
6f87955970
2 changed files with 60 additions and 1 deletions
53
resources/sql/autopatches/20170316.rawfiles.01.php
Normal file
53
resources/sql/autopatches/20170316.rawfiles.01.php
Normal file
|
@ -0,0 +1,53 @@
|
||||||
|
<?php
|
||||||
|
|
||||||
|
// Previously, files generated by "View Raw File" were written as permanent
|
||||||
|
// files with excessively wide view permissions. This destroys these files
|
||||||
|
// so they are recreated as temporary files with correct permissions when
|
||||||
|
// next accessed.
|
||||||
|
|
||||||
|
$table = new DifferentialChangeset();
|
||||||
|
$conn = $table->establishConnection('w');
|
||||||
|
$viewer = PhabricatorUser::getOmnipotentUser();
|
||||||
|
|
||||||
|
$iterator = new LiskRawMigrationIterator(
|
||||||
|
$conn,
|
||||||
|
$table->getTableName());
|
||||||
|
|
||||||
|
echo tsprintf(
|
||||||
|
"%s\n",
|
||||||
|
pht('Purging old raw changeset file caches...'));
|
||||||
|
|
||||||
|
$keys = array(
|
||||||
|
'raw:new:phid',
|
||||||
|
'raw:old:phid',
|
||||||
|
);
|
||||||
|
|
||||||
|
foreach ($iterator as $changeset) {
|
||||||
|
try {
|
||||||
|
$metadata = phutil_json_decode($changeset['metadata']);
|
||||||
|
|
||||||
|
$phids = array();
|
||||||
|
foreach ($keys as $key) {
|
||||||
|
if (isset($metadata[$key])) {
|
||||||
|
$phids[] = $metadata[$key];
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
foreach ($phids as $phid) {
|
||||||
|
$file = id(new PhabricatorFileQuery())
|
||||||
|
->setViewer($viewer)
|
||||||
|
->withPHIDs(array($phid))
|
||||||
|
->executeOne();
|
||||||
|
if ($file) {
|
||||||
|
id(new PhabricatorDestructionEngine())
|
||||||
|
->destroyObject($file);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// NOTE: We don't bother updating the changeset record itself: we already
|
||||||
|
// regenerate the cache properly if the stored value isn't valid.
|
||||||
|
|
||||||
|
} catch (Exception $ex) {
|
||||||
|
// Just move on if we run into trouble.
|
||||||
|
}
|
||||||
|
}
|
|
@ -350,13 +350,19 @@ final class DifferentialChangesetViewController extends DifferentialController {
|
||||||
$data = $changeset->makeOldFile();
|
$data = $changeset->makeOldFile();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$diff = $changeset->getDiff();
|
||||||
|
|
||||||
$file = PhabricatorFile::newFromFileData(
|
$file = PhabricatorFile::newFromFileData(
|
||||||
$data,
|
$data,
|
||||||
array(
|
array(
|
||||||
'name' => $changeset->getFilename(),
|
'name' => $changeset->getFilename(),
|
||||||
'mime-type' => 'text/plain',
|
'mime-type' => 'text/plain',
|
||||||
|
'ttl' => phutil_units('24 hours in seconds'),
|
||||||
|
'viewPolicy' => PhabricatorPolicies::POLICY_NOONE,
|
||||||
));
|
));
|
||||||
|
|
||||||
|
$file->attachToObject($diff->getPHID());
|
||||||
|
|
||||||
$metadata[$key] = $file->getPHID();
|
$metadata[$key] = $file->getPHID();
|
||||||
$changeset->setMetadata($metadata);
|
$changeset->setMetadata($metadata);
|
||||||
$changeset->save();
|
$changeset->save();
|
||||||
|
|
Loading…
Reference in a new issue