revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-24 14:30:56 +01:00
Code Issues Releases Wiki Activity

Restore support for using "arc download" to fetch files with no "security.alternate-file-domain"

Browse source 
Summary:
Fixes T13132. I removed this branch in D19156 when tightening the logic for the new CSP header, but there's a legitimate need for it: downloading files via `arc download`, or more generally being an API consumer of files.

This is not completely safe, but attacks I'm aware of (particularly, cookie fixation, where an attacker could potentially force a victim to become logged in to an account they control) are difficult and not very powerful. We already issue clear setup advice about the importance of configuring this option ("Phabricator is currently configured to serve user uploads directly from the same domain as other content. This is a security risk.") and I think there's significant value in letting API clients just GET file data without having to jump through a lot of weird hoops.

Test Plan:
  - With `security.alternate-file-domain` off, tried to `arc download` a file.
  - Before: downloaded an HTML dialog page.
  - After: downloaded the file.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13132

Differential Revision: https://secure.phabricator.com/D19421
This commit is contained in:
epriestley 2018-05-01 07:08:45 -07:00
parent fb4b9bc2fc
commit 332f4ab66d
1 changed files with 18 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

19
src/applications/files/controller/PhabricatorFileDataController.php
View file

@ -84,12 +84,29 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
$response->setMimeType($file->getViewableMimeType()); $response->setMimeType($file->getViewableMimeType());
} else { } else {
$is_post = $request->isHTTPPost(); $is_post = $request->isHTTPPost();
$is_public = !$viewer->isLoggedIn();
// NOTE: Require POST to download files from the primary domain. If the // NOTE: Require POST to download files from the primary domain. If the
// request is not a POST request but arrives on the primary domain, we // request is not a POST request but arrives on the primary domain, we
// render a confirmation dialog. For discussion, see T13094. // render a confirmation dialog. For discussion, see T13094.
$is_safe = ($is_alternate_domain || $is_lfs || $is_post); // There are two exceptions to this rule:
// Git LFS requests can download with GET. This is safe (Git LFS won't
// execute files it downloads) and necessary to support Git LFS.
// Requests with no credentials may also download with GET. This
// primarily supports downloading files with `arc download` or other
// API clients. This is only "mostly" safe: if you aren't logged in, you
// are likely immune to XSS and CSRF. However, an attacker may still be
// able to set cookies on this domain (for example, to fixate your
// session). For now, we accept these risks because users running
// Phabricator in this mode are knowingly accepting a security risk
// against setup advice, and there's significant value in having
// API development against test and production installs work the same
// way.
$is_safe = ($is_alternate_domain || $is_post || $is_lfs || $is_public);
if (!$is_safe) { if (!$is_safe) {
return $this->newDialog() return $this->newDialog()
->setSubmitURI($file->getDownloadURI()) ->setSubmitURI($file->getDownloadURI())