XSS in footnote text

This should do it, check_markup applied (also to the value in case). The test coverage updates are mostly because that switches <b> tags to <strong> tags, but also covers stripping out an unordered list (which is not allowed in the 'footnote' text format) so that should protect against any future regression there.

The check_markup() doc says:

this function should only be used when filtering text for use elsewhere than on a rendered HTML page. If this is part of an HTML page, then a renderable array with a #type 'processed_text' element should be used instead of this

So I think we should use something like this:

      'text' => [
        '#type' => 'processed_text',
        '#text' => $text,
        '#format' => 'footnote',
      ],
      'text_clean' => PlainTextOutput::renderFromHtml($text),

I don't think the format needs to be applied to text_clean since PlainTextOutput::renderFromHtml() removes every tag anyway.

Hi @prudloff,

I think what that comment is suggesting though is actually what we are doing; ie, rendering for elsewhere other than the page. We do this to collapse multiple footnotes together + create the ref_id (which people could anchor link to). If we instead do not render it, we could change the behaviour of either. The code in check_markup is ultimately doing the same thing, just also triggering Renderer::renderInIsolation() to get the string version earlier on. Nervous to otherwise cause further side effects for a fairly well used functionality.

Ie, we might change the behaviour of this bit

if ($this->settings['footnotes_collapse']) {
  $key = $this->getMatchingFootnoteKey($text);
}

If we e.g. switch to comparing the text_clean version (which still then at least requires that to be rendered early).

So I think its better to merge this and if you feel strongly that we should not render as early as we are currently doing, we can carry on in a follow-up.

Thanks,
Scott

Oh right, I didn't see that the module does a string comparison with the footnote text.

The main problem with check_markup() is that it looses cache metadata or attached libraries generated by the rendered text (because it uses renderInIsolation()).
So if one of the filters used in the Footnote text format adds cache metadata or attached libraries, this metadata will be lost.
For example the "Caption images" filter adds the filter/caption CSS library to the page but if I enable this filter on the Footnote text format, this library is not added to the page.

A solution would be to do the same thing as check_markup but with render() instead:

    $build = [
      '#type' => 'processed_text',
      '#text' => $text,
      '#format' => 'footnote',
    ];
    \Drupal::service('renderer')->render($build);

This way cache metadata and assets correctly bubble to the page.

Makes sense; I updated the code. Rendering $value was breaking tests e.g. ::testCitationUniqueTextSameValue() but I can see its already running through Html::cleanCssIdentifier() if not is_numeric, so I believe that is safe. Did a tiny bit of scope creep to rename a variable to avoid confusion with the variable name being checked to auto-collapse multiple identical texts together.

I tested the MR and confirm it fixes the vulnerability.

Thank you!