From 355b753df70c223019d5490e8dcb5c14dd8a8122 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 1 Aug 2011 21:01:37 -0700
Subject: [PATCH] Prevent file download without POST + CSRF

Summary: This prevents <applet /> attacks unless the attacker can upload an
applet which has a viewable MIME type as detected by `file`. I'm not sure if
this is possible or not. It should, at least, narrow the attack window. There
are no real tradeoffs here, this is probably a strictly better application
behavior regardless of the security issues.
Test Plan:
  - Tried to download a file via GET, got redirected to info.
  - Downloaded a file via POST + CSRF from the info page.

Reviewers: andrewjcg, erling, aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 759
---
 .../controller/list/PhabricatorFileListController.php | 11 +----------
 .../controller/view/PhabricatorFileViewController.php | 10 ++++++++++
 src/applications/files/controller/view/__init__.php   |  1 +
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/applications/files/controller/list/PhabricatorFileListController.php b/src/applications/files/controller/list/PhabricatorFileListController.php
index 1aef4aea2f..6cf4d8f0f4 100644
--- a/src/applications/files/controller/list/PhabricatorFileListController.php
+++ b/src/applications/files/controller/list/PhabricatorFileListController.php
@@ -96,7 +96,7 @@ class PhabricatorFileListController extends PhabricatorFileController {
         phutil_render_tag(
           'a',
           array(
-            'href' => $file->getViewURI(),
+            'href' => $file->getBestURI(),
           ),
           phutil_escape_html($file->getName())),
         phutil_escape_html(number_format($file->getByteSize()).' bytes'),
@@ -108,13 +108,6 @@ class PhabricatorFileListController extends PhabricatorFileController {
           ),
           'Info'),
         $view_button,
-        phutil_render_tag(
-          'a',
-          array(
-            'class' => 'small button grey',
-            'href'  => '/file/download/'.$file->getPHID().'/',
-          ),
-          'Download'),
         phabricator_date($file->getDateCreated(), $user),
         phabricator_time($file->getDateCreated(), $user),
       );
@@ -130,7 +123,6 @@ class PhabricatorFileListController extends PhabricatorFileController {
         'Size',
         '',
         '',
-        '',
         'Created',
         '',
       ));
@@ -142,7 +134,6 @@ class PhabricatorFileListController extends PhabricatorFileController {
         'right',
         'action',
         'action',
-        'action',
         '',
         'right',
       ));
diff --git a/src/applications/files/controller/view/PhabricatorFileViewController.php b/src/applications/files/controller/view/PhabricatorFileViewController.php
index 47a9e57df3..a5dc977037 100644
--- a/src/applications/files/controller/view/PhabricatorFileViewController.php
+++ b/src/applications/files/controller/view/PhabricatorFileViewController.php
@@ -55,6 +55,16 @@ class PhabricatorFileViewController extends PhabricatorFileController {
           $download = true;
         }
 
+        if ($download) {
+          if (!$request->isFormPost()) {
+            // Require a POST to download files to hinder attacks where you
+            // <applet src="http://phabricator.example.com/file/..." /> on some
+            // other domain.
+            return id(new AphrontRedirectResponse())
+              ->setURI($file->getInfoURI());
+          }
+        }
+
         if ($download) {
           $mime_type = $file->getMimeType();
         } else {
diff --git a/src/applications/files/controller/view/__init__.php b/src/applications/files/controller/view/__init__.php
index f6dbd0e152..afe7459ce5 100644
--- a/src/applications/files/controller/view/__init__.php
+++ b/src/applications/files/controller/view/__init__.php
@@ -9,6 +9,7 @@
 phutil_require_module('phabricator', 'aphront/response/400');
 phutil_require_module('phabricator', 'aphront/response/404');
 phutil_require_module('phabricator', 'aphront/response/file');
+phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/files/controller/base');
 phutil_require_module('phabricator', 'applications/files/storage/file');
 phutil_require_module('phabricator', 'applications/files/storage/transformed');