mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-23 22:10:55 +01:00
When viewing raw file content in Differential, cache it into the File tool
before displaying it Summary: @alok reported a vulnerability where Flash will run carefully-crafted plain text files. When the user requests a raw file, cache it into Files if it isn't already there. Then redirect them to Files. This solves the problem by executing the SWF/TXT with CDN-domain permissions, not content-domain permissions, provided the install is correctly configured. (Followup diff coming to make this more universally true.) NOTE: We'll still show raw data in Diffusion. The barrier to XSS here is much higher (you need commit access) but I'll do something similar there. We aren't vulnerable in Paste, since we already use Files. Test Plan: Clicked "View Old File", "View New File" in an alt-domain configuration, got redirected to a cookie-free domain before being delivered the response. Reviewers: btrahan, alok Reviewed By: btrahan CC: aran, epriestley Differential Revision: https://secure.phabricator.com/D1607
This commit is contained in:
parent
cd651001b6
commit
da1d57b60a
2 changed files with 51 additions and 7 deletions
|
@ -54,9 +54,9 @@ class DifferentialChangesetViewController extends DifferentialController {
|
|||
}
|
||||
switch ($view) {
|
||||
case 'new':
|
||||
return $this->buildRawFileResponse($changeset->makeNewFile());
|
||||
return $this->buildRawFileResponse($changeset, $is_new = true);
|
||||
case 'old':
|
||||
return $this->buildRawFileResponse($changeset->makeOldFile());
|
||||
return $this->buildRawFileResponse($changeset, $is_new = false);
|
||||
default:
|
||||
return new Aphront400Response();
|
||||
}
|
||||
|
@ -256,10 +256,53 @@ class DifferentialChangesetViewController extends DifferentialController {
|
|||
$author_phid);
|
||||
}
|
||||
|
||||
private function buildRawFileResponse($text) {
|
||||
return id(new AphrontFileResponse())
|
||||
->setMimeType('text/plain')
|
||||
->setContent($text);
|
||||
private function buildRawFileResponse(
|
||||
DifferentialChangeset $changeset,
|
||||
$is_new) {
|
||||
|
||||
if ($is_new) {
|
||||
$key = 'raw:new:phid';
|
||||
} else {
|
||||
$key = 'raw:old:phid';
|
||||
}
|
||||
|
||||
$metadata = $changeset->getMetadata();
|
||||
|
||||
$file = null;
|
||||
$phid = idx($metadata, $key);
|
||||
if ($phid) {
|
||||
$file = id(new PhabricatorFile())->loadOneWhere(
|
||||
'phid = %s',
|
||||
$phid);
|
||||
}
|
||||
|
||||
if (!$file) {
|
||||
// This is just building a cache of the changeset content in the file
|
||||
// tool, and is safe to run on a read pathway.
|
||||
$unguard = AphrontWriteGuard::beginScopedUnguardedWrites();
|
||||
|
||||
if ($is_new) {
|
||||
$data = $changeset->makeNewFile();
|
||||
} else {
|
||||
$data = $changeset->makeOldFile();
|
||||
}
|
||||
|
||||
$file = PhabricatorFile::newFromFileData(
|
||||
$data,
|
||||
array(
|
||||
'name' => $changeset->getFilename(),
|
||||
'mime-type' => 'text/plain',
|
||||
));
|
||||
|
||||
$metadata[$key] = $file->getPHID();
|
||||
$changeset->setMetadata($metadata);
|
||||
$changeset->save();
|
||||
|
||||
unset($unguard);
|
||||
}
|
||||
|
||||
return id(new AphrontRedirectResponse())
|
||||
->setURI($file->getBestURI());
|
||||
}
|
||||
|
||||
private function buildLintInlineComments($changeset) {
|
||||
|
|
|
@ -11,8 +11,8 @@ phutil_require_module('arcanist', 'unit/result');
|
|||
phutil_require_module('phabricator', 'aphront/response/400');
|
||||
phutil_require_module('phabricator', 'aphront/response/404');
|
||||
phutil_require_module('phabricator', 'aphront/response/ajax');
|
||||
phutil_require_module('phabricator', 'aphront/response/file');
|
||||
phutil_require_module('phabricator', 'aphront/response/redirect');
|
||||
phutil_require_module('phabricator', 'aphront/writeguard');
|
||||
phutil_require_module('phabricator', 'applications/differential/controller/base');
|
||||
phutil_require_module('phabricator', 'applications/differential/parser/changeset');
|
||||
phutil_require_module('phabricator', 'applications/differential/storage/changeset');
|
||||
|
@ -20,6 +20,7 @@ phutil_require_module('phabricator', 'applications/differential/storage/diffprop
|
|||
phutil_require_module('phabricator', 'applications/differential/storage/inlinecomment');
|
||||
phutil_require_module('phabricator', 'applications/differential/view/changesetdetailview');
|
||||
phutil_require_module('phabricator', 'applications/differential/view/primarypane');
|
||||
phutil_require_module('phabricator', 'applications/files/storage/file');
|
||||
phutil_require_module('phabricator', 'applications/markup/engine');
|
||||
phutil_require_module('phabricator', 'applications/phid/handle/data');
|
||||
phutil_require_module('phabricator', 'infrastructure/diff/engine');
|
||||
|
|
Loading…
Reference in a new issue