Discussion: should we rework or remove unsafeHTML and unsafeSVG

[klebba]

Opening an issue to cover past discussion on this topic — work has already begun in #213 — essentially:

• unsafeHTML and unsafeSVG are necessary for developers who prefer to avoid imperative DOM manipulation
• it's not necessary to avoid imperative DOM manipulation
• the convenience of injecting unsafe markup into an otherwise safe template is debatable
• in our experience unsafeSVG has never been necessary (because unsafeHTML can accomplish the same behavior when <svg></svg> wraps the content)

Thus if we remove unsafe entirely we will (potentially inadvertently) encourage unsafe behavior unknowingly (if innerHTML were called unsafeCrazyNativeFeature we would perhaps feel differently). At the same time, the project philosophy states:

Presume adopters are browser experts already (stay out of their way)

Which could be interpreted as "stay out of my way, I know innerHTML is unsafe!" -or- "stay out of my way, I want to conveniently bind unsafe markup in my declarative template!"

The philosophy also states:

Implement a minimal set of generalized functionality

... and it could be argued that unsafe is not a "minimal" feature. Consider the following:

<textarea id="example1">${unsafe(mySampleCode)}</textarea>

This does not actually touch on the common concern when injecting unsafe HTML — the browser will not actually execute the code within the textarea element — instead it will display the HTML as a string. This snippet is exactly identical:

<textarea id="example2">${mySampleCode}</textarea>

A more sinister example is:

<div id="example3">${unsafe(mySampleCode)}</div>

If we had not used the unsafe directive, the template engine would simply print the markup as as string (by assigning div#example3.textContent = mySampleCode — but this is not what we want. We actually want to allow the browser to expand the string and generate DOM (even if the string contains <script>/* malicious bad stuff */</script> because presumably we are confident that other sanitization has already occurred upstream. (Note: do not make that assumption, it is unsafe! Always weigh the risks of what could go wrong and ensure there are other checks in place where it matters.)

So the conundrum in a nutshell:

• get out of the way of experts who presumably know that they need to do unsafe things
• avoid implementing this thing at all; let the experts do the unsafe thing easily but not declaratively (e.g. facilitate getting a pointer to a DOM injection point where the developer can do something unsafe like div#example4.innerHTML = '<script>/* malicious bad stuff */</script>'

Open for discussion...

[klebba]

Rationale for removing:

After seven years and 10,000 commits across 12 engineers there are still only two places where we use unsafeHTML today:

• rendering code in app-code
• rendering markdown in markdown-renderer

Both render calls are with DOMPurify.sanitize in the loop. It would be trivial to rework these integrations to no longer rely on unsafeHTML

---

Rationale for keeping:

In order to hook into the DOM nodes generated by the template engine and its opaque lifecycle developers must be super familiar with XElement which defies an on-deck philosophy idea:

Please do not require that developers learn new proprietary stuff unless it is absolutely necessary

[klebba]

Current exemplary case:

// ...

static template(html, { unsafe }) {
  return ({ markdownResult }) => html`<div id="container">${unsafe(markdownResult)}</div>`;
}

// ...

---

Would become (today):

// ...

static template(html) {
  return () => html`<div id="container"></div>`;
}

render() {
  super.render();
  const target = this.shadowRoot.getElementById('container');
  target?.innerHTML = this.internal.markdownResult ?? '';
}

// ...

Problem with this: render is called far too frequently rather than selectively when markdownResult changes (too clumsy)

---

Another method which is a bit more idiomatic (today):

// ...

static get properties() {
 return {
   markdownResult: {
     type: String,
     compute: // ...
     default: '',
     observe: (host, value) => {
       const target = host.shadowRoot.getElementById('container');
       target?.innerHTML = value;
     },
   },
 };
}

static template(html) {
  return () => html`<div id="container"></div>`;
}

// ...

Problem with this: developers need to know a lot about XElement property features and behavior. Also if #container itself changes we have no way of knowing that it is necessary to reassign the new node with innerHTML.

In both cases developers need to juggle awareness of the property features and the template engine lifecycle and behavior (and have confidence that defying these abstractions is acceptable...)

[klebba]

Maybe the way to go is that we allow for binding of documentFragment or Node objects within a template context e.g.:

// ...

static get properties() {
 return {
   markdownResult: {
     type: String,
     compute: // ...
   },

   markdownFragment: {
     type: DocumentFragment,
     input: ['markdownResult'],
     compute: (markdownResult) => {
       if (markdownResult) {
         return new DocumentFragment().innerHTML = markdownResult;
       }
     },
   },
 };
}

static template(html) {
  return ({ markdownFragment }) => html`<div id="container">${markdownFragment}</div>`;
}

// ...

... essentially what #207 is covering