diff --git a/core/lib/Drupal/Core/Asset/AssetGroupSetHashTrait.php b/core/lib/Drupal/Core/Asset/AssetGroupSetHashTrait.php index e9f41cd..3666605 100644 --- a/core/lib/Drupal/Core/Asset/AssetGroupSetHashTrait.php +++ b/core/lib/Drupal/Core/Asset/AssetGroupSetHashTrait.php @@ -8,13 +8,13 @@ trait AssetGroupSetHashTrait { /** - * Generate a hash for an asset group set. + * Generate a hash for an array of asset groups. * * @param array $groups - * A set of asset groups. + * An array of asset groups. * * @return string - * A hash to uniquely identify the set. + * A hash to uniquely identify the groups. */ protected function generateHash(array $groups) { $normalized = $groups; diff --git a/core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy.php b/core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy.php index e43c127..19ac573 100644 --- a/core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy.php +++ b/core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy.php @@ -15,7 +15,7 @@ class CssCollectionOptimizerLazy implements AssetCollectionGroupOptimizerInterfa use CssCollectionOptimizerTrait; /** - * The grouper for js assets. + * The grouper for CSS assets. * * @var \Drupal\Core\Asset\CssCollectionGrouper */ @@ -70,23 +70,19 @@ public function __construct(AssetCollectionGrouperInterface $grouper, AssetOptim /** * {@inheritdoc} * - * The cache file name is retrieved on a page load via a lookup variable that - * contains an associative array. The array key is the hash of the file names - * in $css while the value is the cache file name. The cache file is generated - * in two cases. First, if there is no file name value for the key, which will - * happen if a new file name has been added to $css or after the lookup - * variable is emptied to force a rebuild of the cache. Second, the cache file - * is generated if it is missing on disk. Old cache files are not deleted - * immediately when the lookup variable is emptied, but are deleted after a - * configurable period (@code system.performance.stale_file_threshold @endcode) - * to ensure that files referenced by a cached page will still be available. + * File names are generated based on library/asset definitions. This includes + * a hash of the assets and the group index. Additionally the full set of + * libraries, already loaded libraries and theme are sent as query parameters + * to allow a PHP controller to generate a valid file with sufficient + * information. Files are not generated by this method since they're assumed + * to be successfully returned from the URL created whether on disk or not. */ public function optimize(array $css_assets) { // Group the assets. $css_groups = $this->grouper->group($css_assets); $key = $this->generateHash($css_groups); - $css_assets = array(); + $css_assets = []; $libraries = []; foreach ($css_groups as $order => $css_group) { // We have to return a single asset, not a group of assets. It is now up @@ -122,16 +118,16 @@ public function optimize(array $css_assets) { break; } } - // Generate a URL each group of assets, but do not process them inline, this - // is done using self::optimizeGroup() when the asset path is requested. - $ajax_page_state = $this->requestStack->getCurrentRequest()->query->get('ajax_page_state'); + // Generate a URL for each group of assets, but do not process them inline, + // this is done using optimizeGroup() when the asset path is requested. + $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state'); $already_loaded = isset($ajax_page_state) ? explode(',', $ajax_page_state['libraries']) : []; - $query_parameters = [ + $query_args = [ 'theme' => $this->themeManager->getActiveTheme()->getName(), 'libraries' => $this->dependencyResolver->getMinimalRepresentativeSubset($libraries), 'already_loaded' => $this->dependencyResolver->getMinimalRepresentativeSubset($already_loaded), ]; - $query = UrlHelper::buildQuery($query_parameters); + $query = UrlHelper::buildQuery($query_args); foreach ($css_assets as $order => $css_asset) { if (!empty($css_asset['preprocessed'])) { $filename = 'css' . '_' . $order . '_' . $key . '.css'; diff --git a/core/lib/Drupal/Core/Asset/JsCollectionOptimizerLazy.php b/core/lib/Drupal/Core/Asset/JsCollectionOptimizerLazy.php index 523bf5a..1082970 100644 --- a/core/lib/Drupal/Core/Asset/JsCollectionOptimizerLazy.php +++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizerLazy.php @@ -148,11 +148,4 @@ public function optimize(array $js_assets) { return $js_assets; } - /** - * {@inheritdoc} - */ - public function getAll() { - return []; - } - } diff --git a/core/lib/Drupal/Core/Asset/JsCollectionOptimizerTrait.php b/core/lib/Drupal/Core/Asset/JsCollectionOptimizerTrait.php index b61543c..9bea1a8 100644 --- a/core/lib/Drupal/Core/Asset/JsCollectionOptimizerTrait.php +++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizerTrait.php @@ -32,7 +32,7 @@ * @return array * The optimized string for the group. */ - public function optimizegroup(array $group) { + public function optimizeGroup(array $group) { $data = ''; foreach ($group['items'] as $js_asset) { // Optimize this JS file, but only if it's not yet minified. @@ -42,8 +42,8 @@ public function optimizegroup(array $group) { else { $data .= $this->optimizer->optimize($js_asset); } - // Append a ';' and a newline after each JS file to prevent them - // from running together. + // Append a ';' and a newline after each JS file to prevent them from + // running together. $data .= ";\n"; } // Remove unwanted JS code that cause issues. diff --git a/core/modules/system/src/Controller/AssetControllerBase.php b/core/modules/system/src/Controller/AssetControllerBase.php index f385d60..d56c59e 100644 --- a/core/modules/system/src/Controller/AssetControllerBase.php +++ b/core/modules/system/src/Controller/AssetControllerBase.php @@ -34,14 +34,14 @@ protected $libraryDependencyResolver; /** - * The Asset Resolver. + * The asset resolver. * * @var \Drupal\Core\Asset\AssetResolverInterface */ protected $assetResolver; /** - * The Theme Initialization. + * The theme initialization. * * @var Drupal\Core\Theme\ThemeInitializationInterface; */ @@ -155,82 +155,80 @@ public function __construct(LibraryDependencyResolverInterface $library_dependen * Thrown when the filename is invalid. */ public function deliver(Request $request, $file_name) { + $uri = 'public://' . $this->assetType . '/' . $file_name; + + // Check to see whether a file matching the $uri already exists, this can + // happen if it was created while this request was in progress. + if (file_exists($uri)) { + return new BinaryFileResponse($uri, 200, ['Cache-control' => $this->cacheControl]); + } + + // First validate that the request is valid enough to produce an asset group + // aggregate. The theme must be passed as a query parameter, since assets + // always depend on the current theme. if (!$request->query->has('theme')) { throw new BadRequestHttpException('The theme must be passed as a query argument'); } - $file_parts = explode('_', basename($file_name, '.' . $this->fileExtension)); + // The group delta is the second segment of the filename and the hash is the // third segment. If either are not there, then the filename is invalid. if (!isset($file_parts[1]) || !is_numeric($file_parts[1]) || !isset($file_parts[2])) { throw new BadRequestHttpException('Invalid filename'); } $group_delta = $file_parts[1]; + $received_hash = $file_parts[2]; + // Now build the asset groups based on the libraries. It requires the full + // set of asset groups to extract and build the aggregate for the group we + // want, since libraries may be split across different asset groups. $theme = $request->query->get('theme'); $active_theme = $this->themeInitialization->initTheme($theme); $this->themeManager->setActiveTheme($active_theme); - // While the libraries are taken from the query parameter, the URL as a - // whole is validated against a hash of the CSS assets later on. $attached_assets = new AttachedAssets(); $attached_assets->setLibraries($request->query->get('libraries')); if ($request->query->has('already_loaded')) { $attached_assets->setAlreadyLoadedLibraries($request->query->get('already_loaded')); } $groups = $this->getGroups($attached_assets, $request); - $key = $this->generateHash($groups); + + // Generate a hash based on the asset groups, this uses the same method as + // the collection optimizer does to create the filename, so it should match. + $generated_hash = $this->generateHash($groups); $group = $this->getGroup($groups, $group_delta); - $hash = $file_parts[2]; + $data = $this->optimizer->optimizeGroup($group); - // The hash from the library definitions in code may not match the hash from - // the URL. This can be for three reasons: + // However, the hash from the library definitions in code may not match the + // hash from the URL. This can be for three reasons: // 1. Someone has requested an outdated URL, i.e. from a cached page, which // matches a different version of the code base. // 2. Someone has requested an outdated URL during a deployment. This is // the same case as #1 but a much shorter window. // 3. Someone is attempting to craft an invalid URL in order to conduct a // denial of service attack on the site. - $match = TRUE; - - $uri = 'public://' . $this->assetType . '/' . $file_name; - if ($key !== $hash) { - // The file requested may have been written to disk by the time we got - // here. If it hasn't, and the hashes don't match, it's possible that a - // file matching the code base for this request already exists on disk, so - // use the filename matching this code base, not the one that generated - // the original filename for the request. - if (!file_exists($uri)) { - $uri = 'public://' . $this->assetType . '/' . $this->assetType . '_' . $group_delta . '_' . $hash . '.' . $this->fileExtension; - } - // Either way, we didn't get a match. - $match = FALSE; - } - if (!file_exists($uri)) { - $data = $this->optimizer->optimizeGroup($group); - // Dump the optimized CSS for this group into an aggregate file. - if ($match) { - $uri = $this->dumper->dumpToUri($data, $this->assetType, $uri); - $state_key = 'drupal_' . $this->assetType . '_cache_files'; - $files = $this->state->get($state_key, []); - $files[] = $uri; - $this->state->set($state_key, $files); - } - // Headers sent from PHP can never perfectly match those sent when the - // file is served by the filesystem, so ensure this request does not get - // cached in either the browser or reverse proxies. Subsequent requests - // for the file will be served from disk and be cached. This is done to - // avoid situations such as where one CDN endpoint is serving a version - // cached from PHP, while another is serving a version cached from disk. - // Should there be any discrepancy in behaviour between those files, this - // can make debugging very difficult. - $response = new Response($data, 200, ['Cache-control' => $this->cacheControl, 'Content-Type' => $this->contentType]); - return $response; - } - else { - return new BinaryFileResponse($uri, 200, ['Cache-control' => $this->cacheControl]); + // Dump the optimized group into an aggregate file, but only if the + // received hash and generated hash match. This prevents invalid filenames + // from filling the disk, while still serving aggregates that may be + // referenced in cached HTML. + if ($received_hash == $generated_hash) { + $uri = $this->dumper->dumpToUri($data, $this->assetType, $uri); + $state_key = 'drupal_' . $this->assetType . '_cache_files'; + $files = $this->state->get($state_key, []); + $files[] = $uri; + $this->state->set($state_key, $files); } + // Headers sent from PHP can never perfectly match those sent when the + // file is served by the filesystem, so ensure this request does not get + // cached in either the browser or reverse proxies. Subsequent requests + // for the file will be served from disk and be cached. This is done to + // avoid situations such as where one CDN endpoint is serving a version + // cached from PHP, while another is serving a version cached from disk. + // Should there be any discrepancy in behaviour between those files, this + // can make debugging very difficult. + $response = new Response($data, 200, ['Cache-control' => $this->cacheControl, 'Content-Type' => $this->contentType]); + return $response; } /** diff --git a/core/modules/system/src/Routing/AssetRoutes.php b/core/modules/system/src/Routing/AssetRoutes.php index 5355653..c7d2de0 100644 --- a/core/modules/system/src/Routing/AssetRoutes.php +++ b/core/modules/system/src/Routing/AssetRoutes.php @@ -8,7 +8,7 @@ use Symfony\Component\Routing\Route; /** - * Defines a routes callback to register a url for serving assets. + * Defines a routes callback to register a url for serving assets. */ class AssetRoutes implements ContainerInjectionInterface { diff --git a/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php index 20afd0b..ea26b9b 100644 --- a/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php +++ b/core/tests/Drupal/FunctionalTests/Asset/AssetOptimizationTest.php @@ -43,6 +43,10 @@ public function testAssetAggregation() { $this->assertAggregate($url, FALSE); } + foreach ($urls as $url) { + $this->assertInvalidAggregates($url); + } + $elements = $page->findAll('xpath', '//script'); $urls = []; foreach ($elements as $element) { @@ -56,13 +60,15 @@ public function testAssetAggregation() { foreach ($urls as $url) { $this->assertAggregate($url, FALSE); } + foreach ($urls as $url) { + $this->assertInvalidAggregates($url, 'js'); + } } protected function assertAggregate($url, $from_php = TRUE) { $url = $this->getAbsoluteUrl($url); $session = $this->getSession(); $session->visit($url); - $page = $session->getPage(); $this->assertResponse(200); $headers = $session->getResponseHeaders(); if ($from_php) { @@ -73,4 +79,61 @@ protected function assertAggregate($url, $from_php = TRUE) { } } + protected function assertInvalidAggregates($url) { + $session = $this->getSession(); + $session->visit($this->replaceGroupDelta($url)); + $this->assertResponse(400); + + $session->visit($this->setInvalidLibrary($url)); + $this->assertResponse(400); + + $session->visit($this->omitTheme($url)); + $this->assertResponse(400); + + $session->visit($this->replaceGroupHash($url)); + $this->assertResponse(200); + $headers = $session->getResponseHeaders(); + $this->assertEqual($headers['Cache-Control'], ['no-store, private']); + + // And again to confirm it's not cached on disk. + $session->visit($this->replaceGroupHash($url)); + $this->assertResponse(200); + $headers = $session->getResponseHeaders(); + $this->assertEqual($headers['Cache-Control'], ['no-store, private']); + + } + + protected function replaceGroupDelta($url) { + $parts = explode('_', $url); + $parts[1] = 100; + return $this->getAbsoluteUrl(implode('_', $parts)); + } + + protected function replaceGroupHash($url) { + $parts = explode('_', $url); + $hash = strtok($parts[2], '.'); + $parts[2] = str_replace($hash, 'abcdefghijklmnop', $parts[2]); + return $this->getAbsoluteUrl(implode('_', $parts)); + } + + protected function setInvalidLibrary($url) { + // First replace the hash so we don't get served the actual file on disk. + $url = $this->replaceGroupHash($url); + $parts = UrlHelper::parse($url); + $parts['query']['libraries'] = ['system/llama']; + + $query = UrlHelper::buildQuery($parts['query']); + return $this->getAbsoluteUrl($parts['path'] . '?' . $query . '#' . $parts['fragment']); + } + + protected function omitTheme($url) { + // First replace the hash so we don't get served the actual file on disk. + $url = $this->replaceGroupHash($url); + $parts = UrlHelper::parse($url); + unset($parts['query']['theme']); + $query = UrlHelper::buildQuery($parts['query']); + return $this->getAbsoluteUrl($parts['path'] . '?' . $query . '#' . $parts['fragment']); + } + + }