Avoid implicit innerHTML assignments

[blois]

Since innerHTML assignments can lead to unintended side effects it should be clear to consumers of these APIs when data may lead to an innerHTML assignment.

An example of an assignment which should probably be clearer:

import ipywidgets as widgets
widgets.Checkbox(description="<marquee>this is html</marquee>")

Potential ways to address this:

• Switch to textContent assignments. This is the easiest to audit and in many cases may be what users expect.
• Use a sanitizer to sanitize the text before assigning to innerHTML. Good for cases where you want to allow rich content but not arbitrary code execution.
• Require an opt-in to use innerHTML or to disable sanitization.

From a quick glance, a few places that could be worth changing:

• ipywidgets/packages/controls/src/widget_bool.ts

  Line 89 in ab54ea0

  this.descriptionSpan.innerHTML = description;

• ipywidgets/packages/controls/src/widget_description.ts

  Line 78 in ab54ea0

  this.label.innerHTML = description;

• ipywidgets/packages/controls/src/widget_selection.ts

  Line 463 in ab54ea0

  button.innerHTML = item_html;

• ipywidgets/packages/controls/src/widget_string.ts

  Line 458 in ab54ea0

  this.datalist.innerHTML = optLines.join('\n');

[jasongrout]

Thanks. This has also bothered me for a long time too.

[jasongrout]

I think a nontrivial fraction of users appreciate being able to do simple styling (bold, italics, colors, etc.) on things like descriptions or checkbox. The widget_selection example is sanitized a few lines above.

[jasongrout]

Require an opt-in to use innerHTML or to disable sanitization.

I like this thought. I think the vast majority of cases are served by textContent. It's nice to have some formatting capability when needed, and for backwards compatibility at this point. I think we probably always ought to sanitize in controls, and have the opt-in to using innerHTML.

[jasongrout]

Is there anyone willing to push this forward in the next week or so for 8.0?

[zerline]

Is there anyone willing to push this forward in the next week or so for 8.0?

If nobody more qualified is available, I can find the time.

[blois]

I was hoping to be able to get a chance to work on this but have not been able to carve out any time yet.

I agree that a default of sanitizing with an option to skip sanitization would be a reasonable approach.

[jasongrout]

Do you think we ought to default to using textContent, or innerHTML with sanitization and perhaps very limited whitelist (like font weight, colors, italics, underline, etc.)?

[blois]

On a clean implementation I'd definitely recommend textContent as it avoids surprises when the text happens to contain chars such as <. Since this is a breaking change it depends on how often people are using the formatting capabilities.

[jasongrout]

@zerline - you may be the one I know that is most using formatting in descriptions, so it would be especially great to hear your thoughts.

Here is one idea: switch to using textContent by default for descriptions (so no formatting, but no surprises either, just plain text), and have a new description_html flag that when it is true, switches to using innerHTML (hopefully with some sanitization)?

[zerline]

@zerline - you may be the one I know that is most using formatting in descriptions

actually I don't, as my inputs usually have no description ..

so it would be especially great to hear your thoughts.

From my experience as a user and developer: We sometimes see icons within input descriptions. Or links to a footnote (anchors). Styling options should be accepted too (<i>, <b>, ... and <style>)

[vidartf]

Did #2785 close this?

[blois]

Yes- great work here, thanks @zerline!