feat: escape =

[gurgunday]

I thought, like Handlebars, escaping ` (on top of ' and ") would help us prevent damage, in case an attacker manages to write JS on a page, by removing the ability to create template literals (as well as string literals), but none of them actually do anything useful in this case because String.fromCharCode() exists

Consider the following scenario where the user erroneously omits quotes from the alt attribute:

const imgAlt = (request) => {
  return html`
    <img src="https://picsum.photos/200/300" alt=${request.query.alt} />
  `

If we click on the link http://127.0.0.1:5000/?alt=altText%20onload=alert(String.fromCharCode(112,119,110,101,100)), we'll get an alert that says "pwned", so creating and using strings is indeed possible despite escaping all types of quotes

And since ` doesn't have a special meaning in HTML either, it seems like escaping it is not useful at all

But removing it could be breaking, and if we're going to release a new major, we better make sure to improve the design, which means having utility functions like htmlAttr instead of 1 function that tries to do everything

For now, I propose also escaping =, which prevents the creation of new attributes like onload= altogether, and for what it's worth, aligns the escaping behavior of ghtml with that of Handlebars

I didn't observe a significant drop in performance

Users should stick to the common knowledge of always quoting their HTML attributes, as stated under the Security section of our README

[gurgunday]

Cc @lirantal, do you know a case where escaping ` is useful?

[github-actions]

Benchmark Results Comparison (140202a):
PR Branch:

[
{
  "Task Name": "simple HTML formatting",
  "ops/sec": "15,602,025",
  "Average Time (ns)": 64.09424058125273,
  "Margin": "±0.10%",
  "Samples": 7801013
},
{
  "Task Name": "null and undefined expressions",
  "ops/sec": "9,194,294",
  "Average Time (ns)": 108.763103341316,
  "Margin": "±0.05%",
  "Samples": 4597148
},
{
  "Task Name": "single string expression",
  "ops/sec": "8,445,016",
  "Average Time (ns)": 118.41303286716965,
  "Margin": "±0.06%",
  "Samples": 4222509
},
{
  "Task Name": "multiple string expressions",
  "ops/sec": "6,490,420",
  "Average Time (ns)": 154.0732205076267,
  "Margin": "±0.06%",
  "Samples": 3245211
},
{
  "Task Name": "array expressions",
  "ops/sec": "706,576",
  "Average Time (ns)": 1415.2739598452788,
  "Margin": "±0.92%",
  "Samples": 353289
},
{
  "Task Name": "array expressions with escapable chars",
  "ops/sec": "325,079",
  "Average Time (ns)": 3076.1674603179663,
  "Margin": "±0.94%",
  "Samples": 162540
},
{
  "Task Name": "object expressions",
  "ops/sec": "6,718,701",
  "Average Time (ns)": 148.83829019350352,
  "Margin": "±0.07%",
  "Samples": 3359351
},
{
  "Task Name": "multiple types of expressions",
  "ops/sec": "934,290",
  "Average Time (ns)": 1070.3304748406733,
  "Margin": "±0.48%",
  "Samples": 467146
},
{
  "Task Name": "large strings",
  "ops/sec": "66,354",
  "Average Time (ns)": 15070.566158298032,
  "Margin": "±0.08%",
  "Samples": 33178
},
{
  "Task Name": "unescaped expressions",
  "ops/sec": "4,925,952",
  "Average Time (ns)": 203.00640971985908,
  "Margin": "±0.11%",
  "Samples": 2462978
},
{
  "Task Name": "escaped expressions",
  "ops/sec": "484,876",
  "Average Time (ns)": 2062.3799594958396,
  "Margin": "±0.55%",
  "Samples": 242439
},
{
  "Task Name": "mixed escaped and unescaped expressions",
  "ops/sec": "1,670,578",
  "Average Time (ns)": 598.5951861024392,
  "Margin": "±0.16%",
  "Samples": 835290
}
]

Main Branch:

[
{
  "Task Name": "simple HTML formatting",
  "ops/sec": "15,226,642",
  "Average Time (ns)": 65.67436094243924,
  "Margin": "±0.06%",
  "Samples": 7613322
},
{
  "Task Name": "null and undefined expressions",
  "ops/sec": "9,164,496",
  "Average Time (ns)": 109.11674311021974,
  "Margin": "±0.06%",
  "Samples": 4582249
},
{
  "Task Name": "single string expression",
  "ops/sec": "8,381,781",
  "Average Time (ns)": 119.30637828569198,
  "Margin": "±0.06%",
  "Samples": 4190891
},
{
  "Task Name": "multiple string expressions",
  "ops/sec": "6,492,805",
  "Average Time (ns)": 154.01661993286228,
  "Margin": "±0.06%",
  "Samples": 3246403
},
{
  "Task Name": "array expressions",
  "ops/sec": "646,371",
  "Average Time (ns)": 1547.0971576746092,
  "Margin": "±0.85%",
  "Samples": 323186
},
{
  "Task Name": "array expressions with escapable chars",
  "ops/sec": "283,146",
  "Average Time (ns)": 3531.7390975763337,
  "Margin": "±12.09%",
  "Samples": 141574
},
{
  "Task Name": "object expressions",
  "ops/sec": "6,857,031",
  "Average Time (ns)": 145.8357029690751,
  "Margin": "±0.65%",
  "Samples": 3428516
},
{
  "Task Name": "multiple types of expressions",
  "ops/sec": "896,534",
  "Average Time (ns)": 1115.4062012022555,
  "Margin": "±0.68%",
  "Samples": 448268
},
{
  "Task Name": "large strings",
  "ops/sec": "66,021",
  "Average Time (ns)": 15146.688800696116,
  "Margin": "±0.14%",
  "Samples": 33011
},
{
  "Task Name": "unescaped expressions",
  "ops/sec": "5,018,053",
  "Average Time (ns)": 199.28047406420586,
  "Margin": "±0.13%",
  "Samples": 2509027
},
{
  "Task Name": "escaped expressions",
  "ops/sec": "472,932",
  "Average Time (ns)": 2114.464754068884,
  "Margin": "±0.42%",
  "Samples": 236467
},
{
  "Task Name": "mixed escaped and unescaped expressions",
  "ops/sec": "1,579,067",
  "Average Time (ns)": 633.2853404664,
  "Margin": "±0.16%",
  "Samples": 789534
}
]

[mcollina]

lgtm

[gurgunday]

Yeah, I can confirm now that the only other reason to escape ` is Internet Explorer, I guess we can keep it for now and be aligned with Handlebars

First, as Hacker News users DanBlake and nbpoole pointed out in a discussion of this blog post, Internet Explorer treats ` as an attribute delimiter. It may be an edge case, but it’s still a potential attack vector, so ` needs to be escaped too.

https://wonko.com/post/html-escaping/

[lirantal]

Just now catching up on this, apologies for the delay.

FWIW, your example about missing to quote attributes, like the following alt is also vulnerable example in React, and I don't think they are taking any specific extra-work to protect against it:

    <img src="https://picsum.photos/200/300" alt=${request.query.alt} />