Follow-up to #2564283: Remove use of SafeMarkup::checkPlain() from adminSummary() and adminLabel() in views plugins

Part of #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping

While trying to reproduce double escaping from #2564283: Remove use of SafeMarkup::checkPlain() from adminSummary() and adminLabel() in views plugins I found an actual alert(), which is rare consider we escape things like gangbusters.

  1. Steps, add a plain text single value "field_alert" with <script>alert('Title');</script> in every field possible on article.
  2. Go to the admin content view and add that new field as a filter.
  3. As soon as you click the checkbox, boom right in the kisser

Will try to figure out where that is coming from.

CommentFileSizeAuthor
#2 escaped_markup_with-2567673-2.patch591 bytesjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
FileSize
escaped_markup_with-2567673-2.patch591 bytes

Here's a fix. use $.html() so that the escaped HTML value is gathered instead of the preresentation when attempting to put back into HTML (alternative is to re-escape in JS)

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Issue tags: +Security
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
Status: Needs review » Reviewed & tested by the community

Tested this manually using the steps to reproduce in the issue summary. I can reproduce the problem and this patch fixes the issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: escaped_markup_with-2567673-2.patch, failed testing.

Status: Needs work » Needs review

lauriii queued 2: escaped_markup_with-2567673-2.patch for re-testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer and at Druid commented
Status: Needs review » Reviewed & tested by the community

It seems like PIFR is having a hard time.. Very unrelated test fails because of this is a JS only patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Yeah .text() is super dangerous will escaped text. This is not the first time see #2513646: Role name is unescaped on block admin via JS and #2512478: XSS on field edit form via label field via ckeditor.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Committed dc2be10 and pushed to 8.0.x. Thanks!

  • alexpott committed dc2be10 on 8.0.x 
    Issue #2567673 by joelpittet: Escaped markup with $.text() injected into...
chx’s picture

chx CreditAttribution: chx commented

> Yeah .text() is super dangerous will escaped text. This is not the first time

Wow! Why is this issue, an actual security hole not critical...? If this is not the first time, what are we doing, API, documentation, i-do-not-know to avoid a next time / contrib / custom?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

For this particular issue you need "administer $entity_type fields", which is at least restricted, but yeah I totally agree that $.text() is a BFM (big fucking mess).

Status: Fixed » Closed (fixed)

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