revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-01-31 17:08:22 +01:00
Code Issues Releases Wiki Activity

Improve consistency of file access policies, particularly for LFS

Browse source 
Summary:
Ref T7789. Currently, we use different viewers if you have `security.alternate-file-domain` configured vs if you do not.

This is largely residual from the days of one-time-tokens, and can cause messy configuration-dependent bugs like the one in T7789#172057.

Instead, always use the omnipotent viewer. Knowledge of the secret key alone is sufficient to access a file.

Test Plan:
  - Disabled `security.alternate-file-domain`.
  - Reproduced an issue similar to the one described on T7789.
  - Applied change.
  - Clean LFS interaction.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D15784
This commit is contained in:
epriestley 2016-04-22 04:40:17 -07:00
parent 711f13660e
commit ab20f243b3
1 changed files with 16 additions and 6 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

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

@ -22,25 +22,21 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
$req_domain = $request->getHost(); $req_domain = $request->getHost();
$main_domain = id(new PhutilURI($base_uri))->getDomain(); $main_domain = id(new PhutilURI($base_uri))->getDomain();
if (!strlen($alt) || $main_domain == $alt_domain) { if (!strlen($alt) || $main_domain == $alt_domain) {
// No alternate domain. // No alternate domain.
$should_redirect = false; $should_redirect = false;
$use_viewer = $viewer;
$is_alternate_domain = false; $is_alternate_domain = false;
} else if ($req_domain != $alt_domain) { } else if ($req_domain != $alt_domain) {
// Alternate domain, but this request is on the main domain. // Alternate domain, but this request is on the main domain.
$should_redirect = true; $should_redirect = true;
$use_viewer = $viewer;
$is_alternate_domain = false; $is_alternate_domain = false;
} else { } else {
// Alternate domain, and on the alternate domain. // Alternate domain, and on the alternate domain.
$should_redirect = false; $should_redirect = false;
$use_viewer = PhabricatorUser::getOmnipotentUser();
$is_alternate_domain = true; $is_alternate_domain = true;
} }
$response = $this->loadFile($use_viewer); $response = $this->loadFile();
if ($response) { if ($response) {
return $response; return $response;
} }
@ -112,7 +108,21 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
return $response; return $response;
} }
private function loadFile(PhabricatorUser $viewer) { private function loadFile() {
// Access to files is provided by knowledge of a per-file secret key in
// the URI. Knowledge of this secret is sufficient to retrieve the file.
// For some requests, we also have a valid viewer. However, for many
// requests (like alternate domain requests or Git LFS requests) we will
// not. Even if we do have a valid viewer, use the omnipotent viewer to
// make this logic simpler and more consistent.
// Beyond making the policy check itself more consistent, this also makes
// sure we're consitent about returning HTTP 404 on bad requests instead
// of serving HTTP 200 with a login page, which can mislead some clients.
$viewer = PhabricatorUser::getOmnipotentUser();
$file = id(new PhabricatorFileQuery()) $file = id(new PhabricatorFileQuery())
->setViewer($viewer) ->setViewer($viewer)
->withPHIDs(array($this->phid)) ->withPHIDs(array($this->phid))