Problem/Motivation

Twig's |raw filter is a pathway to XSS bugs. Core's only usage of it is in time.html.twig. But since we have autoescaping, it's completely unnecessary.

Proposed resolution

  • Remove the 'html' property from the 'time' theme function
  • Remove |raw from the time.html.twig template and html flag and it's docblocks.
  • Remove any 'html' variable reference indication from preprocess.

Remaining tasks

n/a

User interface changes

n/a

API changes

n/a

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it's cleaning up leftover from D7 conversion
Issue priority Major because it's a security issue
Unfrozen changes Unfrozen because it only changes markup and strings
Prioritized changes The main goal of this issue is security
CommentFileSizeAuthor
#9 interdiff.txt1.91 KBjoelpittet
#9 remove_html_option-2500747-9.patch5.2 KBjoelpittet
time.autoescape.patch3.95 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Title: Remove 'html' option for theme('time') » Remove 'html' option from theme('time')
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet for Drupal Association commented
Priority: Normal » Major
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +D8 Accelerate

Completely agree. Thank you for removing this vestige:)

Updated IS and added beta eval. Bumped to major as it's security related.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer and for Drupal Association commented
Issue summary: View changes
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer and for Drupal Association commented
Issue summary: View changes
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer and for Drupal Association commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work

I think there are docs that need an update in core/themes/classy/templates/field/time.html.twig?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Needs work » Reviewed & tested by the community

Err. Disregard that. I failed to actually apply the patch before checking.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work

Oh no, I was right. This bit:

 * - text: (optional) The content to display within the <time> element. Set
 *   'html' to TRUE if this value is already sanitized for output in HTML.
 *   Defaults to a human-readable representation of the timestamp value or the
 *   datetime attribute value using format_date().

So let's remove that reference too. :)

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer and for Drupal Association commented
Status: Needs work » Reviewed & tested by the community
FileSize
remove_html_option-2500747-9.patch5.2 KB
interdiff.txt1.91 KB

Removed two comment leftover references to HTML flag on time.html.twig, back to RTBC.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

+1, changes look good.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thanks for the quick update!

This issue only changes markup and docs, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. It's also prioritized as a security hardening. Committed and pushed to 8.0.x.

  • xjm committed 405f138 on 8.0.x 
    Issue #2500747 by joelpittet, effulgentsia: Remove 'html' option from...

Status: Fixed » Closed (fixed)

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