Problem/Motivation

The ManagedFile form element plugin makes use of a #prefix to render out status messages in Drupal\file\Element\ManagedFile::uploadAjaxCallback. This means that any output from the status message template will be ran through Drupal's XSS filter -- which is known to cause problems.

Steps to reproduce

  1. Set up an off-the-shelf standard install.
  2. Add the webform module (for convenience; a native custom code FAPI form would work just as well...)
  3. Add a required managed file element to a form.
  4. Submit the form without uploading a file.
  5. Note that the status message has an <svg> sprite.
    The Olivero system message template outputs an SVG sprite.
  6. Now things get interesting; upload a file that's too large for the field to accept...
  7. Note that the status message that's rendered by the managed file element does not have an <svg> sprite! The design has been broken!
    System messages rendered by managed file elements strip SVG sprites (amongst other things)

Given this does break the Olivero design, I feel this is a legitimate bug that should be resolved.

Proposed resolution

By the time the status message template has finished rendering, this output should be trusted enough not to need this degree of XSS filtering [citation needed]. Can this be done safely?

Remaining tasks

Ask the security team to chime in.

User interface changes

TBD

API changes

TBD

Data model changes

TBD

Release notes snippet

TBD

CommentFileSizeAuthor
olivery-system-message-no-sprite.png26.28 KBluke.leber
olivero-system-message-sprite.png7.54 KBluke.leber

Comments

Luke.Leber created an issue. See original summary.

cilefen’s picture

cilefen commented

This seems similar in nature to #3455356: Support displaying a message that correctly validates file uploads for the Gin theme.. Do they have a common cause?

luke.leber’s picture

luke.leber
Primary language English
Location Pennsylvania
commented

I believe that the root cause is that Drupal's XSS utility is somewhat dated. There are a pile of "safe" HTML5 tags that are stripped out -- a subset of SVG's elements among them.

This is a somewhat heavy-handed approach that immediately comes to mind to resolve this in the mean-time:

    $status_messages = ['#type' => 'status_messages'];
    
    // Rather than concatenating things in `#prefix`, extract the
    // existing prefix and only run XSS filter on that.
    $prefix = $form['#prefix'] ?? '';
    if (!($prefix instanceof MarkupInterface)) {
      $prefix = Xss::filterAdmin($prefix); // <-- Note - same logic as exists in the renderer service.
    }
    
    // Then render a build array noting that...
    //  1. $prefix has been sanitized.
    //  2. $status_messages has been sanitized (via Twig)
    $build = [
      ['#markup' => Markup::create($prefix)],
      $status_messages,
      array_filter($form, static fn($key) => $key !== '#prefix', ARRAY_FILTER_USE_KEY),
    ];
    $output = $renderer->renderRoot($build);
naveenvalecha’s picture

naveenvalecha
He/Him
commented
Related issues: +#3407067: message.js should work with whitespace in empty message wrappers

This looks related

quietone’s picture

quietone commented
Version: 11.0.x-dev » 11.x-dev

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.

fago’s picture

fago
Primary language German
Location Vienna
commented

I've opened #3618230: Adopt the ajax MessageCommand for file uploads what ports the message rendering over to the Drupal ajax command we have, and that way, also fix this bug. Please take a look.