From 43935d5916f25756f620ca77056a648e5a65176c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 20 Apr 2016 15:14:35 -0700
Subject: [PATCH] Don't cache resources we can't generate properly

Summary:
Fixes T10843. In a multi-server setup, we can do this:

  - Two servers, A and B.
  - You push an update.
  - A gets pushed first.
  - After A has been pushed, but before B has been pushed, a user loads a page from A.
  - It generates resource URIs like `/stuff/new/package.css`.
  - Those requests hit B.
  - B doesn't have the new resources yet.
  - It responds with old resources.
  - Your CDN caches things. You now have a poisoned CDN: old data is saved in a new URL.

To try to avoid this with as little work as possible and generally make it hard to get wrong, check the URL hash against the hash we would generate.

If they don't match, serve our best guess at the resource, but don't cache it. This should make things mostly keep working during the push, but prevent caches from becoming poisoned, and everyone should get a working version of everything after the push finishes.

Test Plan:
  - `curl`'d a resource, got a cacheable one.
  - Changed the hash a little, `curl`'d again. This time: valid resource, but not cacheable.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10843

Differential Revision: https://secure.phabricator.com/D15775
---
 .../celerity/CelerityResourceMap.php          |  5 ++++
 .../CelerityPhabricatorResourceController.php |  6 +++-
 .../controller/CelerityResourceController.php | 29 +++++++++++++------
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/src/applications/celerity/CelerityResourceMap.php b/src/applications/celerity/CelerityResourceMap.php
index 03ccfe607d..e53b656aaf 100644
--- a/src/applications/celerity/CelerityResourceMap.php
+++ b/src/applications/celerity/CelerityResourceMap.php
@@ -208,6 +208,11 @@ final class CelerityResourceMap extends Phobject {
   }
 
 
+  public function getHashForName($name) {
+    return idx($this->nameMap, $name);
+  }
+
+
   /**
    * Return the absolute URI for a resource, identified by hash.
    * This method is fairly low-level and ignores packaging.
diff --git a/src/applications/celerity/controller/CelerityPhabricatorResourceController.php b/src/applications/celerity/controller/CelerityPhabricatorResourceController.php
index c5f878b61e..5751996db8 100644
--- a/src/applications/celerity/controller/CelerityPhabricatorResourceController.php
+++ b/src/applications/celerity/controller/CelerityPhabricatorResourceController.php
@@ -31,7 +31,11 @@ final class CelerityPhabricatorResourceController
       return new Aphront400Response();
     }
 
-    return $this->serveResource($this->path);
+    return $this->serveResource(
+      array(
+        'path' => $this->path,
+        'hash' => $this->hash,
+      ));
   }
 
   protected function buildResourceTransformer() {
diff --git a/src/applications/celerity/controller/CelerityResourceController.php b/src/applications/celerity/controller/CelerityResourceController.php
index 4e6feeac6c..debb37c1ca 100644
--- a/src/applications/celerity/controller/CelerityResourceController.php
+++ b/src/applications/celerity/controller/CelerityResourceController.php
@@ -24,7 +24,10 @@ abstract class CelerityResourceController extends PhabricatorController {
 
   abstract public function getCelerityResourceMap();
 
-  protected function serveResource($path, $package_hash = null) {
+  protected function serveResource(array $spec) {
+    $path = $spec['path'];
+    $hash = idx($spec, 'hash');
+
     // Sanity checking to keep this from exposing anything sensitive, since it
     // ultimately boils down to disk reads.
     if (preg_match('@(//|\.\.)@', $path)) {
@@ -40,18 +43,24 @@ abstract class CelerityResourceController extends PhabricatorController {
 
     $dev_mode = PhabricatorEnv::getEnvConfig('phabricator.developer-mode');
 
-    if (AphrontRequest::getHTTPHeader('If-Modified-Since') && !$dev_mode) {
+    $map = $this->getCelerityResourceMap();
+    $expect_hash = $map->getHashForName($path);
+
+    // Test if the URI hash is correct for our current resource map. If it
+    // is not, refuse to cache this resource. This avoids poisoning caches
+    // and CDNs if we're getting a request for a new resource to an old node
+    // shortly after a push.
+    $is_cacheable = ($hash === $expect_hash) &&
+                    $this->isCacheableResourceType($type);
+    if (AphrontRequest::getHTTPHeader('If-Modified-Since') && $is_cacheable) {
       // Return a "304 Not Modified". We don't care about the value of this
       // field since we never change what resource is served by a given URI.
       return $this->makeResponseCacheable(new Aphront304Response());
     }
 
-    $is_cacheable = (!$dev_mode) &&
-                    $this->isCacheableResourceType($type);
-
     $cache = null;
     $data = null;
-    if ($is_cacheable) {
+    if ($is_cacheable && !$dev_mode) {
       $cache = PhabricatorCaches::getImmutableCache();
 
       $request_path = $this->getRequest()->getPath();
@@ -61,8 +70,6 @@ abstract class CelerityResourceController extends PhabricatorController {
     }
 
     if ($data === null) {
-      $map = $this->getCelerityResourceMap();
-
       if ($map->isPackageResource($path)) {
         $resource_names = $map->getResourceNamesForPackageName($path);
         if (!$resource_names) {
@@ -117,7 +124,11 @@ abstract class CelerityResourceController extends PhabricatorController {
       $response->addAllowOrigin('*');
     }
 
-    return $this->makeResponseCacheable($response);
+    if ($is_cacheable) {
+      $response = $this->makeResponseCacheable($response);
+    }
+
+    return $response;
   }
 
   public static function getSupportedResourceTypes() {