From ab20f243b34183cf55fb1c2a84739b5c5c17c0d0 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 22 Apr 2016 04:40:17 -0700
Subject: [PATCH] Improve consistency of file access policies, particularly for
 LFS

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
---
 .../PhabricatorFileDataController.php         | 22 ++++++++++++++-----
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php
index dfae99d201..5c2fc7bbda 100644
--- a/src/applications/files/controller/PhabricatorFileDataController.php
+++ b/src/applications/files/controller/PhabricatorFileDataController.php
@@ -22,25 +22,21 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
     $req_domain = $request->getHost();
     $main_domain = id(new PhutilURI($base_uri))->getDomain();
 
-
     if (!strlen($alt) || $main_domain == $alt_domain) {
       // No alternate domain.
       $should_redirect = false;
-      $use_viewer = $viewer;
       $is_alternate_domain = false;
     } else if ($req_domain != $alt_domain) {
       // Alternate domain, but this request is on the main domain.
       $should_redirect = true;
-      $use_viewer = $viewer;
       $is_alternate_domain = false;
     } else {
       // Alternate domain, and on the alternate domain.
       $should_redirect = false;
-      $use_viewer = PhabricatorUser::getOmnipotentUser();
       $is_alternate_domain = true;
     }
 
-    $response = $this->loadFile($use_viewer);
+    $response = $this->loadFile();
     if ($response) {
       return $response;
     }
@@ -112,7 +108,21 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
     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())
       ->setViewer($viewer)
       ->withPHIDs(array($this->phid))