Follow-up to #2568605: Replace remaining !placeholder for Non-URL HTML outputs only in template_preprocess_comment()

Follow-up to #2568603: Replace remaining !placeholder for Non-URL HTML outputs only in core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php

Problem/Motivation

See #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only

Theme name stuff in SystemController

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 2568607.2.patch3.54 KBalexpott

Comments

dawehner created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
Parent issue: #2568605: Replace remaining !placeholder for Non-URL HTML outputs only in template_preprocess_comment() »
StatusFileSize
new 2568607.2.patch3.54 KB
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

The escaping of the title is alright.
While testing I realized that the escaping of the description doesn't work, well changing the .info file already means being able to change some other files certainly could do much more.

xjm’s picture

xjm
she/her
Primary language English
commented

@dawehner, how did you test it?

@alexpott said of this one:

Theme::getName check plains this so check plaining is fine

but I didn't investigate that.

dawehner’s picture

dawehner
Primary language German
commented

Well, I hacked <script>alert(123);</script> into bartik for the name of the theme ... and yeah the description is neither escaped nor XSS filtered (which it should be probably), but its a problem of this particular issue.

xjm’s picture

xjm
she/her
Primary language English
commented

Right, I remember discussing a related issue (whereby module developers could put XSS in their .info.yml) earlier this summer. But agreed that using @placeholder for the theme name here is reasonable, especially if it's consistent with HEAD (and not double-escaped) based on your manual testing. And that the rest of the escaping (or not) for this extension metadata is out of scope here.

For automated test coverage, I don't makes any sense to add tests for the single (and not double) escaping of special characters in a theme name, since it's not user input and arguably not even valid in the first place for it to contain special characters. So committed and pushed to 8.0.x. Thanks!

  • xjm committed 872cc61 on 8.0.x 
    Issue #2568607 by alexpott, dawehner: Replace remaining !placeholder for...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Parent issue: » #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.