Toast: Escape toast HTML content that is not wrapped in "-pharos-" tag

[ymouzakis]

User supplied strings can end up being part of a toast messages content. This is a XSS risk.

This change: (check at least one)

• Adds a new feature
• Fixes a bug
• Improves maintainability
• Improves documentation
• Is a release activity

Is this a breaking change? (check one)

• Yes
• No

Is the: (complete all)

• Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
• Test suite(s) passing?
• Code coverage maximal?
• Changeset added?
• Component status page up to date?

What does this change address?
A clear and concise description or a direct link to an issue [e.g. #15]
If adding a feature, a rationale for its addition goes here.

How does this change work?
A clear, detailed description of how this change addresses the issue.
This could be a bullet list if there are several moving parts.

Additional context
Add any other context here.

[changeset-bot]

🦋 Changeset detected

Latest commit: fa7d891

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@ithaka/pharos	Major
@ithaka/pharos-site	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[michael-iden]

I wonder if theres any way we could feel comfortable making this a patch release? It might not be possible but that will lower the barrier of entry to release the update

[ymouzakis]

I can change it. I was erroring on the side of caution since this could be a breaking change.

[michael-iden]

I could see us having a brief discussion with the group. I can definitely see where you were coming from as things could break but is it OK that they break if it fixes a semi-vulnerability?

[daneah]

I have concerns about this change, which amount to:

• It should generally be the responsibility of the consumer to ensure escaping of untrusted HTML content rather than rendering it to the browser. Cleaning HTML content here in Pharos does not ensure that the untrusted content isn't rendered somewhere else in some other way, and may in fact deliver surprising results to developers who will need to read the Toast code to understand why they're seeing what they're seeing.
• Choosing this one place to avoid rendering untrusted content feels like a narrowly-scoped pattern that should be avoided; we would want to apply this kind of protection across many components if we wanted to go this direction, and I don't think the solution as-is can be iteratively expanded on to get to that approach since it's fairly surgical in nature. A broader useful pattern that could be more integrated would be necessary if we wanted to broadly apply protection from untrusted HTML.

Ultimately my recommendation would be to perform HTML escaping at the consumer level prior to applying Pharos for rendering components, whether server-side or in other client-side code.

[ymouzakis]

@daneah Sounds reasonable to me. My thinking was to fix it here rather than depending on each consumer to clean HTML, but I hear where you're coming from.

[brentswisher]

@daneah I agree that this pattern of using a library to manually sanitize HTML is generally problematic and isn't a pattern we want to adopt. I do however wonder if there is a middle ground here.

To your point about consumers being responsible for escaping content, many modern frontend frameworks (React, Vue) do provide built-in escaping of HTML to prevent XSS out of the box, and don't make it a responsibility of their consumer. Content is made safe by default and something like dangerouslySetInnerHTML is required if you want to pass it in and not have it be escaped. I'm curious if Lit does that as well?

From what I remember from the report which surfaced this, the same content that is causing issues in the Toast is displayed elsewhere on the page via Pharos components without the potential XSS, which makes me think it might. (@michael-iden does that sound right?)

From what I can tell, this instance in the Toast component is the only place in the codebase that the element.innerHTML function is used to set the content directly. Perhaps if we were to refactor it to use the normal output methods of Lit like the other components, we could get Lit's default handling of content, without needing to manually sanitize the output?

I haven't had a chance to look into it to verify my assumptions, but I'm curious if that makes any sense to you and is worth looking into?

[daneah]

@brentswisher good points and yes, I think that would be worth looking into! I am remembering some strife with using slots here, but as usual the details have been lost to the annals of time in my brain 😓

[ymouzakis]

Right now the effort to deal with this pseudo-vulnerability is not really worth it in out opinion. Maybe we can come back to this later. For now I am closing this PR.