Plan to fix all current CVEs

[Pierstoval]

As of this statement in #7:

I haven't checked, but are the current CVEs resolved on the fork?

They are related to XSS flaws:

• CVE-2019-11002(tooltip component)
• CVE-2019-11003(autocomplete component)
• CVE-2019-11004(toast component)

We know the CVEs are not fixed yet on Materialize, and there are already discussions in the original repository (Dogfalo#6331 and Dogfalo#6286) about this subject.

Current situation

Since the current CVEs are related to XSS that can be triggered when Materialize is used in a bad manner, this is the developer's responsibility to sanitize their data before using the three aforementioned components.

However, since the warnings will still display, and since Materialize can be "considered harmful" because of these (even though the discussions on the original repo suggest that it's not Materialize's problem, but rather how devs are using it), we should still find solutions to fix the warnings in an easy and backwards-compatible manner.

This is important for some CI that don't accept dependencies with warnings, or for clients that are worried about warnings they might not understand, especially when it's written "XSS" in them (which is a well-known flaw on the web in general)

Proposal

I suggest that for all the three involved components we add a boolean sanitizeInput option.

When this option is true, the input data will be sanitized with an equivalent of htmlspecialchars() in PHP. There are plenty examples about this on the internet, the best one I found is this one here:

function escapeHtml(text) {
  var map = {'&': '&amp;', '<': '&lt;', '>': '&gt;', '"': '&quot;', "'": '&#039;'};
  
  return text.replace(/[&<>"']/g, function(m) { return map[m]; });
}

Suggestions on other sanitizing methods are encouraged, since this has never been a "standard" process in the web industry 😄 .

Behavior on Materialize 1.x

This option will by default be null. To keep backwards compatibility, null will be equivalent to false.

However, if we detect null, we display a warning in the console, as a reminder to the developer that the native behavior of not-sanitizing is deprecated, and default value will change in 2.x to true.
To remove the warning, we suggest the user to specify the value themselves, even if it's false.
An explicit false will not trigger the warning, since this means that the developer knows what they are doing, and in such case, Materialize cannot guarantee input data safety.

Behavior on the future Materialize 2.x branch

Here, default value is true, and any falsy value will be interpreted as false. We could add type checks to force the user to specify a boolean, but I think this might be overkill. Suggestions appreciated 😉

---

Feel free to discuss all the different points, and even provide PRs if necessary.

[DanielRuf]

I would suggest DOMPurify which is also used by others and Bootstrap mentions it at https://getbootstrap.com/docs/4.3/getting-started/javascript/.

I would not write our own sanitizer / reinvent the wheel as this could also lead to other issues like bypasses due to problems in the logic.

https://github.com/cure53/DOMPurify

[Pierstoval]

As long as it doesn't introduce too much overhead/bloat on the dependencies, I feel like it's okay to introduce a new dependency for this 👍

[FINDarkside]

You only need sanitizers if you want to allow HTML. Currently autocomplete does not work and is not indended to work with HTML data. So autocomplete wouldn't need sanitizer, it would simply need to use textContent equivalent instead of innerHTML.

When this option is true, the input data will be sanitized with an equivalent of htmlspecialchars() in PHP.

I just want to emphasise that this does not fix the vulnerability with autocomplete, as demonstrated here. If you don't sanitize the data, <img src=/ onerror="alert(\'xss\')">typethis will trigger XSS, if you sanitize, <img src=/ onerror="alert(\'xss\')">still not safe will trigger it.

Root of the problem is Autocomplete._highlight first calling $el.text() effectively unescaping the provided data and then later setting the results with $el.html.

[samschurter]

@Pierstoval I've been reading through the other discussions on this, including the response from Snyk, and I like the suggestion from @lirantal for Toast. It would be easy (I assume) and non-breaking to add a properly sanitized text property and update the documentation. Make the examples use text by default and document that the html property is unsafe and should only be used with trusted/properly sanitized data. The response from @lirantal says that it's not obvious that Tooltip accepts HTML, but the options are similar to Toast and include an html property. I think that Toast and Tooltip can be fixed with the same non-breaking method. In fact, maybe we add both text and unsafeHTML properties and mark the existing html property as deprecated - to be removed completely in a later release. That would eventually make it completely obvious that passing HTML to a Toast or Tooltip is not safe.

@DanielRuf I think this is worth a slightly breaking change on Autocomplete for the first release of materializecss/materialize for the following reasons:

• This shouldn't affect anyone currently using the old npm repository at materialize-css, only people who are adding the new @materializecss/materialize repository. That means anyone "upgrading" from materialize-css to @materializecss/materialize will have the opportunity to see a message in the README.md about the breaking change.
• It seems the behavior was never intended to include HTML output. Anyone doing that already is essentially exploiting a bug, not using a documented feature. Breaking someone's code by fixing a bug they were exploiting may make them angry, but I don't think we have any responsibility to feel bad about that. Especially when the bug can cause vulnerabilities in code written by inexperienced devs.
• It would be really nice if the first release from this team didn't have any publicly visible vulnerabilities, regardless of whether we believe it should be the developer's or the library's responsibility to sanitize inputs. It's just good public relations and I think that's important for adoption of the new fork.
• The breaking change would force upgrading users injecting HTML in their Autocomplete to review their code and either stop injecting HTML or use a new property or option to allow unsafe HTML.

Thoughts?

I think the change can be minimally breaking by adding a default property to the Autocomplete options like allowUnsafeHTML: false. Then all a developer needs to do to make their existing code work is add allowUnsafeHTML: true to their initialization options. The other path I had thought of would be harder to upgrade, but potentially better. I'm already working on making Autocomplete accept Chips data as well as the standard URL/null. There may be a good way to implement an unsafeHTML property on that data object. Not sure though.

[wuda-io]

Yes thats awesome! It would also be very cool to add IDs to the Autocomplete Data.

[samschurter]

Root of the problem is Autocomplete._highlight first calling $el.text() effectively unescaping the provided data and then later setting the results with $el.html.

In't the code here also a problem?

materialize/js/autocomplete.js

Lines 382 to 386 in a8a30a3

	$autocompleteOption.append(
	`<img src="${entry.data}" class="right circle"><span>${entry.key}</span>`
	);
	} else {
	$autocompleteOption.append('<span>' + entry.key + '</span>');

The image URL could potentially be a user-supplied avatar URL. Is the escaping done by the following sufficient?

img = document.createElement('img'); 
document.body.appendChild(img);
img.src="x\" onerror=\"alert(\'XSS Attack\')\"";
console.log(img.src);
// Output: "x%22%20onerror=%22alert('XSS%20Attack')%22"