Validation of library names fails in AssetControllerBase

Even if we can never generate a URL parameter like this someone could theoretically send one so it's worth accounting for it.

Could use some test coverage - there's existing coverage of invalid asset URLs so should be straightforward to add to that.

Do you happen to know which test this is in? I just took a quick look (not on my laptop right now), but unfortunately couldn't find the right test.

+1 to the overall change and test coverage. LGTM!

1. In the return statement, instead of

an arbitrary string

can we please update this to

empty

/**
 * Replaces the 'include' query parameter with an empty value.
 *
 * @param string $url
 *   The source URL.
 *
 * @return string
 *   The URL with 'include' set to empty.
 */

2. I was just thinking if we can refactor the code duplication in test file.
something like..

/**
 * Creates a URL with an empty query parameter validation.
 *
 * @param string $url
 *   The source URL.
 * @param string $parameter
 *   The query parameter to set as empty ('include' or 'exclude').
 *
 * @return string
 *   The URL with the specified parameter set to empty.
 */
protected function createUrlWithEmptyParameter(string $url, string $parameter): string {
  $url = $this->replaceGroupHash($url);
  $parts = UrlHelper::parse($url);
  $parts['query'][$parameter] = UrlHelper::compressQueryParameter('');
  
  $query = UrlHelper::buildQuery($parts['query']);
  return $this->getAbsoluteUrl($parts['path'] . '?' . $query . $fragment);
}

In terms of readability, the current code is clear and straightforward. So we can ignore this.

Thanks!

Can we clean up the steps some to be more concrete? Will help with the review.

While debugging the test to see why it fails, I noticed that it is impossible for the include parameter to ever be empty (or eJwDAAAAAAE). Even if it passes the validation changed by the MR it would fail shortly with message "Invalid filename" after because the (empty) library definition could not be grouped.
Therefore I removed the test for an empty include parameter.

Thank you @stborchert for providing the steps.
I verified the changes and looks good to me.

Before changes:
https://drupal11-ddev.ddev.site/sites/default/files/css/css_kRHLMdVsfEc-...

After changes:
https://drupal11-ddev.ddev.site/sites/default/files/css/css_kRHLMdVsfEc-...

Thanks!

One comment on the MR - I would have expected the validation to mean there's no need to change any code outside AssetControllerBase.

Therefore I removed the test for an empty include parameter.

I think this coverage is probably worth having - if we change the validation logic, it would prevent a possible regression.

One comment on the MR - I would have expected the validation to mean there's no need to change any code outside AssetControllerBase.

I thought so, too. But unfortunately I ran into an error without the changes:
Exception: Warning: Undefined array key 1 Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies()() (Line: 67)

The other changes do not alter the test results so I reverted them.

Adding the test for empty includes again leads to other errors. You'll get BadRequestHttpException('Invalid filename.') from AssetControllerBase::getGroup() because the "loaded" library groups are empty.

Trying to fix this directly in AssetControllerBase by changing <?php $include_libraries = explode(',', UrlHelper::uncompressQueryParameter($request->query->get('include')));?> to <?php $include_libraries = array_filter(explode(',', UrlHelper::uncompressQueryParameter($request->query->get('include'))));?> or by changing the code in LibraryDependencyResover does not work since later Drupal tries to load the assets for "group with delta 1" (<?php $group = $this->getGroup($groups, $request->query->get('delta')); ?> (AssetControllerBase:181) but since there are no asset groups, this fails the "Invalid filename" exception.

I'm unsure where to fix this. AssetControllerBase::getGroups() correctly does not create groups for empty libraries but if this happens we always run into the "Invalid filename" exception because a non-existing asset group is requested.

I thought so, too. But unfortunately I ran into an error without the changes:
Exception: Warning: Undefined array key 1 Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies()() (Line: 67)

Right but this can only happen based on how it's called from AssetControllerBase, so this suggests the validation in AssetControllerBase still isn't enough?