search

phetsims / chains

This application demonstrates the various types of strings that may appear in PhET simulations, and is intended to be used for testing string-translation tools.
GNU General Public License v3.0
0 stars 2 forks source link

Sim is vulnerable to xss #3

Open phet-steele opened 7 years ago

phet-steele commented 7 years ago

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

jonathanolson commented 7 years ago

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 commented 7 years ago

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

jonathanolson commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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:

Examples from the scenery-phet demo:

pixelzoom commented 7 years ago

In https://github.com/phetsims/chains/issues/3#issuecomment-292111091, 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 commented 7 years ago

I have moved discussion of RichText to https://github.com/phetsims/scenery-phet/issues/300.

zepumph commented 1 year ago

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