Problem/Motivation

In #3416700: Handle invalid compressed ajax_page_state more gracefully we added protection against invalid library query strings, but AttachedAssets is a value object that could validate these internally for all cases.

Steps to reproduce

Proposed resolution

Add validation to ::setLibraries() and ::setAlreadyLoadedLibraries().
Remove the validation from AssetControllerBase::deliver().

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3442980-5.patch872 bytesdieterholvoet
#4 3442980-4.patch605 bytesseanb

Issue fork drupal-3442980

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

longwave created an issue. See original summary.

anybody’s picture

Thanks @longwave! It would be great then to add information about which library broke, because currently this is hard to find out for site managers. They see "The libraries to include are encoded incorrectly." but have no clue which library is encoded incorrectly and what that means. Perhaps "encoded incorrectly" isn't the perfect wording also?

See https://www.drupal.org/project/asset_injector/issues/3444733#comment-155... for details and background.

catch’s picture

I think we should open separate issue to add more information about the library to the error message so we can backport that to 10.2.x (and 10.3.x once 10.3.0 is out) - this issue will make things even stricter so will only be able to go into a minor release. I have to admit I didn't imagine that a module would specify a real library with two forward slashes in it, the validation is supposed to be against people trying to inject malicious information into the query parameter (e.g. disk filling attempts, things like that).

seanb’s picture

StatusFileSize
new605 bytes

Came here via #3348789: Compress ajax_page_state and #3416700: Handle invalid compressed ajax_page_state more gracefully. I also got the Warning: Undefined array key 1 in Drupal\Core\Asset\LibraryDependencyResolver->doGetDependencies() error because system_js_settings_alter ended up returning an empty string. When this string is compressed and unpacked this ends up adding an empty library which can not be resolved.

Regarding the solution, I think it would be best if system_js_settings_alter simply doesn't add $settings['ajaxPageState']['libraries'] when the list of libraries is empty.

So instead of this:

    // Provide the page with information about the individual asset libraries
    // used, information not otherwise available when aggregation is enabled.
    $minimal_libraries = $library_dependency_resolver->getMinimalRepresentativeSubset(array_unique(array_merge(
      $assets->getLibraries(),
      $assets->getAlreadyLoadedLibraries()
    )));
    sort($minimal_libraries);
    $settings['ajaxPageState']['libraries'] = implode(',', $minimal_libraries);

We do this:

    // Provide the page with information about the individual asset libraries
    // used, information not otherwise available when aggregation is enabled.
    $minimal_libraries = $library_dependency_resolver->getMinimalRepresentativeSubset(array_unique(array_merge(
      $assets->getLibraries(),
      $assets->getAlreadyLoadedLibraries()
    )));
    sort($minimal_libraries);
    if (!empty($minimal_libraries)) {
      $settings['ajaxPageState']['libraries'] = implode(',', $minimal_libraries);
    }

I attached a patch for anyone that needs it. I'm not sure if this is the correct issue to address it, I can create a separate issue if needed.

dieterholvoet’s picture

StatusFileSize
new872 bytes

I re-rolled the patch for Drupal 11.1.x.

mayurgajar made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.