From a9f2c07345d041661f66aa2388d0cf2f4bbdf26f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 11 Aug 2014 09:39:25 -0700
Subject: [PATCH] Generate a 403 page with a nice dialog when a file token is
 invalid

Summary:
Ref T5685. Currently we just 403 on an invalid token, but we can be a little more helpful.

The issues here are:

  - If we **do** redirect you on this page and something goes wrong, you might get stuck in a redirect loop.
  - If we **don't** redirect you, copy/pasting the link to someone (or reloading the page) gives them a pretty confusing result, since the link doesn't work any more. Prior to this diff, they get a 403.

To mitigate this, do a little better than a bare 403: give them a link to auth and generate a new URI for the file.

If this is still confusing, the next best thing I can come up with is something like this:

  - Put some modulous of the timestamp in the URI.
  - If the current time is within 2 seconds of the generation time, show this dialog.
  - Otherwise, redirect.

That seems like it would be okay, but I worry that "2" has to be small (so links you copy/paste -> chat -> click still work) and a small value means that a small amount of clock skew breaks things. We could use the database clock, but ehhh.

Other ideas:

  - Put a hash of the remote IP in the URI, redirect if it doesn't match. Fails for companies behind a NAT gateway but should work in a lot of other cases.
  - Just redirect always, there's no reason it should ever loop and browsers don't really do anything bad when there's a loop (they'll show an error after too many redirects).

I'm leaning toward letting this stabilize in the wild for a bit, then trying "always redirect".

Test Plan: {F188914}

Reviewers: btrahan, 20after4

Reviewed By: 20after4

Subscribers: epriestley

Maniphest Tasks: T5685

Differential Revision: https://secure.phabricator.com/D10215
---
 .../controller/PhabricatorFileController.php  | 11 -------
 .../PhabricatorFileDataController.php         | 31 ++++++++++++++++---
 .../PhabricatorFileListController.php         | 11 +++++++
 3 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/src/applications/files/controller/PhabricatorFileController.php b/src/applications/files/controller/PhabricatorFileController.php
index a77525c84f..bbdbd21e07 100644
--- a/src/applications/files/controller/PhabricatorFileController.php
+++ b/src/applications/files/controller/PhabricatorFileController.php
@@ -2,17 +2,6 @@
 
 abstract class PhabricatorFileController extends PhabricatorController {
 
-  public function buildApplicationCrumbs() {
-    $crumbs = parent::buildApplicationCrumbs();
-    $crumbs->addAction(
-      id(new PHUIListItemView())
-        ->setName(pht('Upload File'))
-        ->setIcon('fa-upload')
-        ->setHref($this->getApplicationURI('/upload/')));
-
-    return $crumbs;
-  }
-
   protected function buildSideNavView() {
     $menu = $this->buildMenu($for_devices = false);
     return AphrontSideNavFilterView::newFromMenu($menu);
diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php
index d6cb0ff0d6..75b62b5dc1 100644
--- a/src/applications/files/controller/PhabricatorFileDataController.php
+++ b/src/applications/files/controller/PhabricatorFileDataController.php
@@ -90,23 +90,46 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
         return $error_response;
       }
 
+      $acquire_token_uri = id(new PhutilURI($file->getViewURI()))
+        ->setDomain($main_domain);
+
+
       if ($this->token) {
         // validate the token, if it is valid, continue
         $validated_token = $file->validateOneTimeToken($this->token);
 
         if (!$validated_token) {
-          return new Aphront403Response();
+          $dialog = $this->newDialog()
+            ->setShortTitle(pht('Expired File'))
+            ->setTitle(pht('File Link Has Expired'))
+            ->appendParagraph(
+              pht(
+                'The link you followed to view this file is invalid or '.
+                'expired.'))
+            ->appendParagraph(
+              pht(
+                'Continue to generate a new link to the file. You may be '.
+                'required to log in.'))
+            ->addCancelButton(
+              $acquire_token_uri,
+              pht('Continue'));
+
+          // Build an explicit response so we can respond with HTTP/403 instead
+          // of HTTP/200.
+          $response = id(new AphrontDialogResponse())
+            ->setDialog($dialog)
+            ->setHTTPResponseCode(403);
+
+          return $response;
         }
         // return the file data without cache headers
         $cache_response = false;
       } else if (!$file->getCanCDN()) {
         // file cannot be served via cdn, and no token given
         // redirect to the main domain to aquire a token
-        $file_uri = id(new PhutilURI($file->getViewURI()))
-            ->setDomain($main_domain);
 
         return id(new AphrontRedirectResponse())
-          ->setURI($file_uri);
+          ->setURI($acquire_token_uri);
       }
     }
 
diff --git a/src/applications/files/controller/PhabricatorFileListController.php b/src/applications/files/controller/PhabricatorFileListController.php
index 9cd50ccb97..feea6db3a6 100644
--- a/src/applications/files/controller/PhabricatorFileListController.php
+++ b/src/applications/files/controller/PhabricatorFileListController.php
@@ -22,4 +22,15 @@ final class PhabricatorFileListController extends PhabricatorFileController {
     return $this->delegateToController($controller);
   }
 
+  public function buildApplicationCrumbs() {
+    $crumbs = parent::buildApplicationCrumbs();
+    $crumbs->addAction(
+      id(new PHUIListItemView())
+        ->setName(pht('Upload File'))
+        ->setIcon('fa-upload')
+        ->setHref($this->getApplicationURI('/upload/')));
+
+    return $crumbs;
+  }
+
 }