From 2896da384cb7366fbcf41f8488ecea281393dfba Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 4 Apr 2017 09:06:00 -0700
Subject: [PATCH] Only require POST to fetch file data if the viewer is logged
 in

Summary:
Ref T11357. In D17611, I added `file.search`, which includes a `"dataURI"`. Partly, this is building toward resolving T8348.

However, in some cases you can't GET this URI because of a security measure:

  - You have not configured `security.alternate-file-domain`.
  - The file isn't web-viewable.
  - (The request isn't an LFS request.)

The goal of this security mechanism is just to protect against session hijacking, so it's also safe to disable it if the viewer didn't present any credentials (since that means there's nothing to hijack). Add that exception, and reorganize the code a little bit.

Test Plan:
  - From the browser (with a session), tried to GET a binary data file. Got redirected.
  - Got a download with POST.
  - From the CLI (without a session), tried to GET a binary data file. Go a download.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11357

Differential Revision: https://secure.phabricator.com/D17613
---
 .../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 5c2fc7bbda..43b5aa419f 100644
--- a/src/applications/files/controller/PhabricatorFileDataController.php
+++ b/src/applications/files/controller/PhabricatorFileDataController.php
@@ -84,18 +84,28 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
     if ($is_viewable && !$force_download) {
       $response->setMimeType($file->getViewableMimeType());
     } else {
-      if (!$request->isHTTPPost() && !$is_alternate_domain && !$is_lfs) {
-        // NOTE: Require POST to download files from the primary domain. We'd
-        // rather go full-bore and do a real CSRF check, but can't currently
-        // authenticate users on the file domain. This should blunt any
-        // attacks based on iframes, script tags, applet tags, etc., at least.
-        // Send the user to the "info" page if they're using some other method.
+      $is_public = !$viewer->isLoggedIn();
+      $is_post = $request->isHTTPPost();
 
+      // NOTE: Require POST to download files from the primary domain if the
+      // request includes credentials. The "Download File" links we generate
+      // in the web UI are forms which use POST to satisfy this requirement.
+
+      // The intent is to make attacks based on tags like "<iframe />" and
+      // "<script />" (which can issue GET requests, but can not easily issue
+      // POST requests) more difficult to execute.
+
+      // The best defense against these attacks is to use an alternate file
+      // domain, which is why we strongly recommend doing so.
+
+      $is_safe = ($is_alternate_domain || $is_lfs || $is_post || $is_public);
+      if (!$is_safe) {
         // This is marked as "external" because it is fully qualified.
         return id(new AphrontRedirectResponse())
           ->setIsExternal(true)
           ->setURI(PhabricatorEnv::getProductionURI($file->getBestURI()));
       }
+
       $response->setMimeType($file->getMimeType());
       $response->setDownload($file->getName());
     }