diff --git a/src/applications/celerity/CelerityResourceMap.php b/src/applications/celerity/CelerityResourceMap.php --- a/src/applications/celerity/CelerityResourceMap.php +++ b/src/applications/celerity/CelerityResourceMap.php @@ -208,6 +208,11 @@ } + 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 --- a/src/applications/celerity/controller/CelerityPhabricatorResourceController.php +++ b/src/applications/celerity/controller/CelerityPhabricatorResourceController.php @@ -31,7 +31,11 @@ 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 --- a/src/applications/celerity/controller/CelerityResourceController.php +++ b/src/applications/celerity/controller/CelerityResourceController.php @@ -24,7 +24,10 @@ 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 @@ $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 @@ } if ($data === null) { - $map = $this->getCelerityResourceMap(); - if ($map->isPackageResource($path)) { $resource_names = $map->getResourceNamesForPackageName($path); if (!$resource_names) { @@ -117,7 +124,11 @@ $response->addAllowOrigin('*'); } - return $this->makeResponseCacheable($response); + if ($is_cacheable) { + $response = $this->makeResponseCacheable($response); + } + + return $response; } public static function getSupportedResourceTypes() {