html-template-tag icon indicating copy to clipboard operation
html-template-tag copied to clipboard

Array arguments aren't escaped

Open aqueenan opened this issue 4 years ago • 2 comments

If you pass in an array argument, the elements of the array are joined, but the result isn't escaped, so there is an opportunity for script injection.

aqueenan avatar Mar 30 '20 02:03 aqueenan

Hi @aqueenan,

Yes, that's by design as it allows for nested interpolations. Ideally, you would pass every element of the array through the html template tag (or any other function that performs escaping at some level).

// ❌ Danger
var names = ["Some", "Name", "/><script>alert('xss')</script>"];
var string = html`
	<ul>
		${names}
	</ul>
`;
// "<ul>SomeName/><script>alert('xss')</script></ul>"

// ❌ Danger
var names = ["Some", "Name", "/><script>alert('xss')</script>"];
var string = html`
	<ul>
		${names.map((name) => `
			<li>Hello, ${name}!</li>
		`)}
	</ul>
`;
// "<ul><li>Hello, Some!</li><li>Hello, Name!</li><li>Hello, /><script>alert('xss')</script>!</li></ul>"

// ✅ Safe
var names = ["Some", "Name", "/><script>alert('xss')</script>"];
var string = html`
	<ul>
		${names.map((name) => html`
			<li>Hello, ${name}!</li>
		`)}
	</ul>
`;
// "<ul><li>Hello, Some!</li><li>Hello, Name!</li><li>Hello, /&gt;&lt;script&gt;alert(&#39;xss&#39;)&lt;/script&gt;!</li></ul>"

If we were to escape array elements, then you wouldn't be able to have nested interpolations.

// ❌ This wouldn't work if we escaped array elements
var names = ["Some", "Name", "/><script>alert('xss')</script>"];
var string = html`
	<ul>
		${names.map((name) => `
			<li>Hello, ${name}!</li>
		`)}
	</ul>
`;
// "<ul>&lt;li&gt;Hello, Some!&lt;/li&gt;&lt;li&gt;Hello, Name!&lt;/li&gt;&lt;li&gt;Hello, /&gt;&lt;script&gt;alert(&#39;xss&#39;)&lt;/script&gt;!&lt;/li&gt;</ul>"

Ultimately, what the html template tag does is allow handwritten HTML to go through while escaping all the interpolations within that string literal.

AntonioVdlC avatar Apr 11 '20 18:04 AntonioVdlC

Proposal: ${[...]} should escape the contents of the array, and $${[...]} should not escape.

Advantages:

  1. It’s safe by default; you have to opt into the behavior of not escaping by introducing the extra $.
  2. It’s more consistent with the non-array case.

Disadvantage: It’s a breaking change.

leafac avatar Jan 28 '21 15:01 leafac