Don't require one-time tokens to view file resources

Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).

The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.

Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.

Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.

Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.

Test Plan:
  - Configured alternate file domain.
  - Viewed site: stuff worked.
  - Accessed a file on primary domain, got redirected to alternate domain.
  - Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
  - Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
  - Reloaded task, new URI generated transparently.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10262

Differential Revision: https://secure.phabricator.com/D15642

src/__phutil_library_map__.php

@@ -2378,7 +2378,6 @@ phutil_register_library_map(array(
     'PhabricatorFeedStoryPublisher' => 'applications/feed/PhabricatorFeedStoryPublisher.php',
     'PhabricatorFeedStoryReference' => 'applications/feed/storage/PhabricatorFeedStoryReference.php',
     'PhabricatorFile' => 'applications/files/storage/PhabricatorFile.php',
-    'PhabricatorFileAccessTemporaryTokenType' => 'applications/files/temporarytoken/PhabricatorFileAccessTemporaryTokenType.php',
     'PhabricatorFileBundleLoader' => 'applications/files/query/PhabricatorFileBundleLoader.php',
     'PhabricatorFileChunk' => 'applications/files/storage/PhabricatorFileChunk.php',
     'PhabricatorFileChunkIterator' => 'applications/files/engine/PhabricatorFileChunkIterator.php',
@@ -6842,7 +6841,6 @@ phutil_register_library_map(array(
       'PhabricatorPolicyInterface',
       'PhabricatorDestructibleInterface',
     ),
-    'PhabricatorFileAccessTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType',
     'PhabricatorFileBundleLoader' => 'Phobject',
     'PhabricatorFileChunk' => array(
       'PhabricatorFileDAO',

src/aphront/response/AphrontFileResponse.php

@@ -11,7 +11,6 @@ final class AphrontFileResponse extends AphrontResponse {
   private $rangeMin;
   private $rangeMax;
   private $allowOrigins = array();
-  private $fileToken;
 
   public function addAllowOrigin($origin) {
     $this->allowOrigins[] = $origin;
@@ -76,15 +75,6 @@ final class AphrontFileResponse extends AphrontResponse {
     return $this;
   }
 
-  public function setTemporaryFileToken(PhabricatorAuthTemporaryToken $token) {
-    $this->fileToken = $token;
-    return $this;
-  }
-
-  public function getTemporaryFileToken() {
-    return $this->fileToken;
-  }
-
   public function getHeaders() {
     $headers = array(
       array('Content-Type', $this->getMimeType()),
@@ -128,15 +118,4 @@ final class AphrontFileResponse extends AphrontResponse {
     return $headers;
   }
 
-  public function didCompleteWrite($aborted) {
-    if (!$aborted) {
-      $token = $this->getTemporaryFileToken();
-      if ($token) {
-        $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-          $token->delete();
-        unset($unguarded);
-      }
-    }
-  }
-
 }

src/aphront/response/AphrontProxyResponse.php

@@ -39,6 +39,11 @@ abstract class AphrontProxyResponse
     return $this;
   }
 
+  public function setCanCDN($can_cdn) {
+    $this->getProxy()->setCanCDN($can_cdn);
+    return $this;
+  }
+
   public function setLastModified($epoch_timestamp) {
     $this->getProxy()->setLastModified($epoch_timestamp);
     return $this;

src/aphront/response/AphrontResponse.php

@@ -4,6 +4,7 @@ abstract class AphrontResponse extends Phobject {
 
   private $request;
   private $cacheable = false;
+  private $canCDN;
   private $responseCode = 200;
   private $lastModified = null;
 
@@ -66,6 +67,11 @@ abstract class AphrontResponse extends Phobject {
     return $this;
   }
 
+  public function setCanCDN($can_cdn) {
+    $this->canCDN = $can_cdn;
+    return $this;
+  }
+
   public function setLastModified($epoch_timestamp) {
     $this->lastModified = $epoch_timestamp;
     return $this;
@@ -186,6 +192,18 @@ abstract class AphrontResponse extends Phobject {
   public function getCacheHeaders() {
     $headers = array();
     if ($this->cacheable) {
+      if ($this->canCDN) {
+        $headers[] = array(
+          'Cache-Control',
+          'public',
+        );
+      } else {
+        $headers[] = array(
+          'Cache-Control',
+          'private',
+        );
+      }
+
       $headers[] = array(
         'Expires',
         $this->formatEpochTimestampForHTTPHeader(time() + $this->cacheable),
@@ -193,11 +211,7 @@ abstract class AphrontResponse extends Phobject {
     } else {
       $headers[] = array(
         'Cache-Control',
-        'private, no-cache, no-store, must-revalidate',
-      );
-      $headers[] = array(
-        'Pragma',
-        'no-cache',
+        'no-store',
       );
       $headers[] = array(
         'Expires',

src/applications/celerity/controller/CelerityResourceController.php

@@ -140,6 +140,7 @@ abstract class CelerityResourceController extends PhabricatorController {
   private function makeResponseCacheable(AphrontResponse $response) {
     $response->setCacheDurationInSeconds(60 * 60 * 24 * 30);
     $response->setLastModified(time());
+    $response->setCanCDN(true);
 
     return $response;
   }

src/applications/diffusion/controller/DiffusionServeController.php

@@ -991,7 +991,7 @@ final class DiffusionServeController extends DiffusionController {
           // <https://github.com/github/git-lfs/issues/1088>
           $no_authorization = 'Basic '.base64_encode('none');
 
-          $get_uri = $file->getCDNURIWithToken();
+          $get_uri = $file->getCDNURI();
           $actions['download'] = array(
             'href' => $get_uri,
             'header' => array(

src/applications/files/controller/PhabricatorFileDataController.php

@@ -4,7 +4,6 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
 
   private $phid;
   private $key;
-  private $token;
   private $file;
 
   public function shouldRequireLogin() {
@@ -15,7 +14,6 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
     $viewer = $request->getViewer();
     $this->phid = $request->getURIData('phid');
     $this->key = $request->getURIData('key');
-    $this->token = $request->getURIData('token');
 
     $alt = PhabricatorEnv::getEnvConfig('security.alternate-file-domain');
     $base_uri = PhabricatorEnv::getEnvConfig('phabricator.base-uri');
@@ -24,99 +22,40 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
     $req_domain = $request->getHost();
     $main_domain = id(new PhutilURI($base_uri))->getDomain();
 
-    $cache_response = true;
 
-    if (empty($alt) || $main_domain == $alt_domain) {
-      // Alternate files domain isn't configured or it's set
-      // to the same as the default domain
-
-      $response = $this->loadFile($viewer);
-      if ($response) {
-        return $response;
-      }
-      $file = $this->getFile();
-
-      // when the file is not CDNable, don't allow cache
-      $cache_response = $file->getCanCDN();
+    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 is configured but this request isn't using it
+      // 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($viewer);
-      if ($response) {
-        return $response;
-      }
-      $file = $this->getFile();
+    $response = $this->loadFile($use_viewer);
+    if ($response) {
+      return $response;
+    }
 
-      // if the user can see the file, generate a token;
-      // redirect to the alt domain with the token;
-      $token_uri = $file->getCDNURIWithToken();
-      $token_uri = new PhutilURI($token_uri);
-      $token_uri = $this->addURIParameters($token_uri);
+    $file = $this->getFile();
 
+    if ($should_redirect) {
       return id(new AphrontRedirectResponse())
         ->setIsExternal(true)
-        ->setURI($token_uri);
-
-    } else {
-      // We are using the alternate domain. We don't have authentication
-      // on this domain, so we bypass policy checks when loading the file.
-
-      $bypass_policies = PhabricatorUser::getOmnipotentUser();
-      $response = $this->loadFile($bypass_policies);
-      if ($response) {
-        return $response;
-      }
-      $file = $this->getFile();
-
-      $acquire_token_uri = id(new PhutilURI($file->getViewURI()))
-        ->setDomain($main_domain);
-      $acquire_token_uri = $this->addURIParameters($acquire_token_uri);
-
-      if ($this->token) {
-        // validate the token, if it is valid, continue
-        $validated_token = $file->validateOneTimeToken($this->token);
-
-        if (!$validated_token) {
-          $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
-
-        // This is marked as an "external" URI because it is fully qualified.
-        return id(new AphrontRedirectResponse())
-          ->setIsExternal(true)
-          ->setURI($acquire_token_uri);
-      }
+        ->setURI($file->getCDNURI());
     }
 
     $response = new AphrontFileResponse();
-    if ($cache_response) {
-      $response->setCacheDurationInSeconds(60 * 60 * 24 * 30);
-    }
+    $response->setCacheDurationInSeconds(60 * 60 * 24 * 30);
+    $response->setCanCDN($file->getCanCDN());
 
     $begin = null;
     $end = null;
@@ -138,10 +77,6 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
         $response->setHTTPResponseCode(206);
         $response->setRange($begin, ($end - 1));
       }
-    } else if (isset($validated_token)) {
-      // We set this on the response, and the response deletes it after the
-      // transfer completes. This allows transfers to be resumed, in theory.
-      $response->setTemporaryFileToken($validated_token);
     }
 
     $is_viewable = $file->isViewableInBrowser();
@@ -150,7 +85,7 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
     if ($is_viewable && !$force_download) {
       $response->setMimeType($file->getViewableMimeType());
     } else {
-      if (!$request->isHTTPPost() && !$alt_domain) {
+      if (!$request->isHTTPPost() && !$is_alternate_domain) {
         // 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
@@ -174,20 +109,6 @@ final class PhabricatorFileDataController extends PhabricatorFileController {
     return $response;
   }
 
-  /**
-   * Add passthrough parameters to the URI so they aren't lost when we
-   * redirect to acquire tokens.
-   */
-  private function addURIParameters(PhutilURI $uri) {
-    $request = $this->getRequest();
-
-    if ($request->getBool('download')) {
-      $uri->setQueryParam('download', 1);
-    }
-
-    return $uri;
-  }
-
   private function loadFile(PhabricatorUser $viewer) {
     $file = id(new PhabricatorFileQuery())
       ->setViewer($viewer)

src/applications/files/storage/PhabricatorFile.php

@@ -697,10 +697,10 @@ final class PhabricatorFile extends PhabricatorFileDAO
         pht('You must save a file before you can generate a view URI.'));
     }
 
-    return $this->getCDNURI(null);
+    return $this->getCDNURI();
   }
 
-  private function getCDNURI($token) {
+  public function getCDNURI() {
     $name = self::normalizeFileName($this->getName());
     $name = phutil_escape_uri($name);
 
@@ -720,9 +720,6 @@ final class PhabricatorFile extends PhabricatorFileDAO
 
     $parts[] = $this->getSecretKey();
     $parts[] = $this->getPHID();
-    if ($token) {
-      $parts[] = $token;
-    }
     $parts[] = $name;
 
     $path = '/'.implode('/', $parts);
@@ -737,19 +734,6 @@ final class PhabricatorFile extends PhabricatorFileDAO
     }
   }
 
-  /**
-   * Get the CDN URI for this file, including a one-time-use security token.
-   *
-   */
-  public function getCDNURIWithToken() {
-    if (!$this->getPHID()) {
-      throw new Exception(
-        pht('You must save a file before you can generate a CDN URI.'));
-    }
-
-    return $this->getCDNURI($this->generateOneTimeToken());
-  }
-
 
   public function getInfoURI() {
     return '/'.$this->getMonogram();
@@ -1120,38 +1104,6 @@ final class PhabricatorFile extends PhabricatorFileDAO
     return $this;
   }
 
-  protected function generateOneTimeToken() {
-    $key = Filesystem::readRandomCharacters(16);
-    $token_type = PhabricatorFileAccessTemporaryTokenType::TOKENTYPE;
-
-    // Save the new secret.
-    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-      $token = id(new PhabricatorAuthTemporaryToken())
-        ->setTokenResource($this->getPHID())
-        ->setTokenType($token_type)
-        ->setTokenExpires(time() + phutil_units('1 hour in seconds'))
-        ->setTokenCode(PhabricatorHash::digest($key))
-        ->save();
-    unset($unguarded);
-
-    return $key;
-  }
-
-  public function validateOneTimeToken($token_code) {
-    $token_type = PhabricatorFileAccessTemporaryTokenType::TOKENTYPE;
-
-    $token = id(new PhabricatorAuthTemporaryTokenQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withTokenResources(array($this->getPHID()))
-      ->withTokenTypes(array($token_type))
-      ->withExpired(false)
-      ->withTokenCodes(array(PhabricatorHash::digest($token_code)))
-      ->executeOne();
-
-    return $token;
-  }
-
-
   /**
    * Write the policy edge between this file and some object.
    *

src/applications/files/temporarytoken/PhabricatorFileAccessTemporaryTokenType.php

@@ -1,17 +0,0 @@
-<?php
-
-final class PhabricatorFileAccessTemporaryTokenType
-  extends PhabricatorAuthTemporaryTokenType {
-
-  const TOKENTYPE = 'file:onetime';
-
-  public function getTokenTypeDisplayName() {
-    return pht('File Access');
-  }
-
-  public function getTokenReadableTypeName(
-    PhabricatorAuthTemporaryToken $token) {
-    return pht('File Access Token');
-  }
-
-}

src/applications/system/controller/PhabricatorRobotsController.php

@@ -32,6 +32,7 @@ final class PhabricatorRobotsController extends PhabricatorController {
 
     return id(new AphrontPlainTextResponse())
       ->setContent($content)
-      ->setCacheDurationInSeconds(phutil_units('2 hours in seconds'));
+      ->setCacheDurationInSeconds(phutil_units('2 hours in seconds'))
+      ->setCanCDN(true);
   }
 }