Sim is vulnerable to xss

[phet-steele]

Run the sim with ?stringTest=xss to be redirected. Running on current master (4/4 1:30 PM).

[jonathanolson]

It's passing a string directly to HTMLText. @pixelzoom, can we remove the HTMLText part of this for chains, or replace it with subsuptext?

[pixelzoom]

We could replace with SubSupText. But why is this not a general vulnerability for any sim that uses HTMLText?

[jonathanolson]

But why is this not a general vulnerability for any sim that uses HTMLText?

It's very much a general vulnerability if you pass a translated string (i.e. a string that can be controlled by query parameters) to an HTMLText. That's one of the primary reasons for the stringTest=xss testing.

We really need more rich-text support, whether that parses it and uses Text nodes (so it could be rendered in WebGL/etc.), or whether it's validation-based and uses HTMLText (which would reject or reform strings that could be potential exploits).

[pixelzoom]

Rather than replace HTMText with SubSupText in the chains example, it would preferable to fix the vulnerability in HTMLText.

Also noting that that chains should include a SubSupText example, and it currently does not, see #4.

[jonathanolson]

Rather than replace HTMText with SubSupText in the chains example, it would preferable to fix the vulnerability in HTMLText.

It's not a vulnerability in HTMLText directly, as it's designed to allow arbitrary content.

I'm looking into things like https://github.com/ecto/bleach for the browser, which would sanitize the incoming string to remove anything not on a whitelist.

If we have a list of desired rich-text support, it may be more beneficial to extend/generalize SubSupText.

[jonathanolson]

Best solution so far looks to find an HTML parser (e.g. https://github.com/andrejewski/himalaya) and then either construct white-listed HTML from it (rendered with HTMLText), or to use that parsing to do the equivalent of a more-structured SubSupText that handles more markup with Text nodes.

e.g.:

himalaya.parse( '<b>boo <i>who</i></b><sup>2</sup>' );
// gives
[
  {
    "type": "Element",
    "tagName": "b",
    "attributes": {},
    "children": [
      {
        "type": "Text",
        "content": "boo "
      },
      {
        "type": "Element",
        "tagName": "i",
        "attributes": {},
        "children": [
          {
            "type": "Text",
            "content": "who"
          }
        ]
      }
    ]
  },
  {
    "type": "Element",
    "tagName": "sup",
    "attributes": {},
    "children": [
      {
        "type": "Text",
        "content": "2"
      }
    ]
  }
]

tagging for developer meeting discussion (I'll be able to describe the different approaches and their drawbacks).

[jonathanolson]

Documentation on RichText (for discussion during developer meeting):

Displays rich text with HTML-style tags by splitting it into multiple (child) Text nodes.

It should be a close to drop-in replacement for SubSupText, and supports the following markup and features:

• <b> and <strong> for bold text
• <i> and <em> for italic text
• <sub> and <sup> for subscripts / superscripts
• <u> for underlined text
• <s> for strikethrough text
• <font> tags with attributes color="cssString", face="familyString", size="cssSize"
• <span> tags with a dir="ltr" / dir="rtl" attribute
• Unicode bidirectional marks (present in PhET strings) for full RTL support

Examples from the scenery-phet demo:

• new RichText( 'RichText can have <b>bold</b> and <i>italic</i> text.' ),
• new RichText( 'Can do H<sub>2</sub>O (A<sub>sub</sub> and A<sup>sup</sup>), or nesting: x<sup>2<sup>2</sup></sup>' ),
• new RichText( 'Additionally: <font color="blue">color</font>, <font size="30px">sizes</font>, <font face="serif">faces</font>, <s>strikethrough</s>, and <u>underline</u>' ),
• new RichText( 'These <b><em>can</em> <u><font color="red">be</font> mixed<sup>1</sup></u></b>.' ),
• new RichText( '\u202aHandles bidirectional text: \u202b<font color="#0a0">مقابض</font> النص ثنائي <b>الاتجاه</b><sub>2</sub>\u202c\u202c' )

[pixelzoom]

In #3 (comment), it looks like @jonathanolson has created something new, RichText. Highly recommend to create a scenery-phet issue for reviewing that, rather than bury it in this issue.

[pixelzoom]

I have moved discussion of RichText to phetsims/scenery-phet#300.

[zepumph]

@jonathanolson is this still a problem? Ready to close?