Please also credit nlisgo, akalata, nod_, harjotsingh
because #2120113: XHTML is not a thing anymore, remove <!--//--><![CDATA[//><!-- //--><!]]> for escaping inline JS/CSS

Problem/Motivation

#value_prefix and #value_suffix allow CDATA escaping of inline scripts, which is a security risk and hasn't been applicable since XHTML went away (for IE5 one needed the CDATA XML tags to include javascript on a page.

Proposed resolution

Remove code inserting CDATA values to browser prefix and suffix.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#4 2550467.4.patch5.56 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

manningpete created an issue. See original summary.

manningpete’s picture

manningpete CreditAttribution: manningpete commented
Title: Remove prefix from the prefix, and suffix from the suffix » Remove #value_prefix and #value_suffix from HtmlTag
alexpott’s picture

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

As per #2120113: XHTML is not a thing anymore, remove <!--//--><![CDATA[//><!-- //--><!]]> for escaping inline JS/CSS the whole CDATA use case is bogus.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
Issue tags: -D8 Accelerate +Security
Related issues: +#2120113: XHTML is not a thing anymore, remove <!--//--><![CDATA[//><!-- //--><!]]> for escaping inline JS/CSS
FileSize
2550467.4.patch5.56 KB

With this patch the html_tag render element is finally not a sec hole waiting to happen.

manningpete’s picture

manningpete CreditAttribution: manningpete commented
Status: Needs review » Reviewed & tested by the community

I reviewed the patch and verified it removes the CDATA code. Goodbye, 2000!

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

would be good to credit
nlisgo, akalata, nod_, harjotsingh
this issue is doing more, but also also solving #2120113: XHTML is not a thing anymore, remove <!--//--><![CDATA[//><!-- //--><!]]> for escaping inline JS/CSS

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: +Security improvements
manningpete’s picture

manningpete CreditAttribution: manningpete commented
Issue summary: View changes
akalata’s picture

akalata CreditAttribution: akalata commented

Sweet!

alexpott’s picture

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

Adding people to the commit credit

manningpete’s picture

manningpete CreditAttribution: manningpete commented
Assigned: manningpete » Unassigned

  • catch committed 146f084 on 8.0.x 
    Issue #2550467 and #2120113 by alexpott, manningpete, akalata, nlisgo,...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

This looks great.

Going to try a small experiment with d.o issue credit and put both this nid and #2120113: XHTML is not a thing anymore, remove <!--//--><![CDATA[//><!-- //--><!]]> for escaping inline JS/CSS in the commit message.

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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