Currently, asset injector summaries are decoded, however this results in unnecessarily escaped HTML.
e.g. if the summary returns:
$this->t('Condition filter values: %theme.', [
'%theme' => $this->configFactory->get('system.theme')->get('default'),
]);
Use a condition which returns a summary containing Drupal placeholders.
Don't escape as the underlying module providing the summary should've already handled all the necessary sanitization.
We could also potentially wrap it in something along the lines of this (but shouldn't be necessary since it's not user generated content):
Markup::create(Xss::filterAdmin($summary->render()));
Provide issue fork/patches.
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 3348521-asset_injector-dont-decode-summary-3.diff | 1010 bytes | codebymikey |
Show commandsStart 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:
changes, plain diff MR !9
Comments
Comment #3
codebymikey commentedAdded issue fork, the current approach also allows it to work if the third party module happens to return a simple string rather than markup.
Since it's passed through the Drupal renderer anyway, the raw HTML entities will escaped anyway if they haven't been passed in as trusted "Markup".
Comment #5
anybodyThe module's implementation changed in the meantime:
$data['conditions'][$condition_id] = is_string($summary) ? Html::decodeEntities($summary) : Html::decodeEntities($summary->render());Could the maintainer eventually explain why
Html::decodeEntities()is used here at all?I also can't really see a benefit, theoretically it's even more a risk:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Utility%21Html.php/function/Html%3A%3AdecodeEntities/8.9.x So I think it should be removed for good reasons, if it's not needed.Comment #6
pookmish commentedThere was a reason when the code was first written. I think it was something to do with the summary provided by one of the condition plugins not being compatible.
Looking at the git blame brings up this issue: #3385148: Calling ->render()on string throws errors.
But now looking at it, that is an issue with the condition_path module, not this module. However, it's doesn't appear to need to be decoded and leaving the summary un-rendered works fine.
So this seems appropriate to me.
Comment #8
pookmish commented