The technique demonstrated here for producing the markup is bad:
1. The use of DOMPurify is either unnecessary or insufficient. Specifically consider reply.content, which is provided from the server as HTML. (I believe it’s the only one that’s supposed to be serialised HTML; all the other fields are text.) If the backend guarantees that you get a clean DOM, DOMPurify is unnecessary†. But if it’s possible for a user to control the markup in that field completely, then DOMPurify as configured is insufficient, because although it blocks XSS and JavaScript execution, it doesn’t filter out remote resource loading and CSS, which can be almost as harmful. Trivial example, <a href=//malicious.example style=position:fixed;inset:0>pwned</a>. Given the type of content, you probably want to either blacklist quite a few things (e.g. the style attribute, and the img, picture, source, audio and video tags), or whitelist a small set of things.
2. Various fields that could hypothetically contain magic characters are dropped in with no escaping. If they do contain magic characters like < and &, you’ve just messed up the display and opened the way for malicious resource loading and problem № 3 below. Even if they are supposed to be unable to contain a magic character (e.g. I’m going to guess that reply.account.username is safe), it’s probably a good idea to escape them anyway just in case (perhaps an API change later makes it possible), and to guard against errant copy–pasters and editors of the code that don’t know what they’re doing. Perhaps at some point you’ll add or switch to reply.account.display_name, which probably can contain < and &.
3. The markup is produced by mixing static templating with user-provided input, and sanitisation is performed on the whole thing. It’s important when doing this sort of templating that each user-provided input be escaped or sanitised by itself in isolation, not as part of a whole that has been concatenated. Otherwise you can mess with the DOM tree accidentally or deliberately. Suppose, for example, that reply.content could contain `</div></div></div><img src=//ad.example alt="Legitimate-looking in-stream ad"><div class="mastodon-comment">…<div class="content">…<div class="mastodon-comment-content">…`. So this means:
• Apply attributes and text nodes to a real DOM (e.g. `img = new Image(); img.src = reply.account.avatar_static; avatar.append(img)`), or escape them in the HTML serialisation (e.g. `<img src="${reply.account.avatar_static.replace(/&/g, "&").replace(/"/g, """)}">`).
• Do HTML sanitisation on just the user input, e.g. DOMPurify.sanitize(reply.content).
A part of my problem with the code as written is that, purely from looking at the code, I can see that there may well be various security holes. I require knowledge of the backend also before I can judge whether there are security holes. Where possible, it’s best to write the code in such a way that you know that it’s safe, or that it’s not—try not to depend on subtle things like “the particular fields that we access happen to be unable to contain angle brackets, ampersands and quotes”, because they’re fragile.
Incidentally, it would also be more efficient to run DOMPurify with the RETURN_DOM_FRAGMENT option, and append that, rather than concatenating it to a string and setting innerHTML. Saves a pointless serialisation/deserialisation round trip, and avoids any possibility of new mXSS vulnerabilities that might be discovered in the future. (I don’t really understand why DOMPurify defaults to emitting a string. I can’t remember seeing a single non-demo use of DOMPurify where the string is preferable to a DOM fragment.)
—
† Though if the server sanitised arbitrary user-provided HTML/MathML/SVG, I probably don’t trust it as much as I trust DOMPurify, for things that end up in the DOM. There are some pretty crazy subtleties in things like HTML serialisation round-tripping of HTML, SVG and MathML content. There’s fun reading in the changelogs and security patches.
I'm laughing a little about "reply.account.display_name" being safe. Back in the early days of Slashdot, I registered a super l33t username with "|<" in it to replace a "k". Well... turns out that this broke things all over the place, and I was satisfied with my username of "|" showing up everywhere :)
When I wrote my comment, it was using reply.account.username, which I would guess is safe. Now it’s using a properly escaped reply.account.display_name (good job there, ognarb), which I expect could otherwise contain < and &.
1. The use of DOMPurify is either unnecessary or insufficient. Specifically consider reply.content, which is provided from the server as HTML. (I believe it’s the only one that’s supposed to be serialised HTML; all the other fields are text.) If the backend guarantees that you get a clean DOM, DOMPurify is unnecessary†. But if it’s possible for a user to control the markup in that field completely, then DOMPurify as configured is insufficient, because although it blocks XSS and JavaScript execution, it doesn’t filter out remote resource loading and CSS, which can be almost as harmful. Trivial example, <a href=//malicious.example style=position:fixed;inset:0>pwned</a>. Given the type of content, you probably want to either blacklist quite a few things (e.g. the style attribute, and the img, picture, source, audio and video tags), or whitelist a small set of things.
2. Various fields that could hypothetically contain magic characters are dropped in with no escaping. If they do contain magic characters like < and &, you’ve just messed up the display and opened the way for malicious resource loading and problem № 3 below. Even if they are supposed to be unable to contain a magic character (e.g. I’m going to guess that reply.account.username is safe), it’s probably a good idea to escape them anyway just in case (perhaps an API change later makes it possible), and to guard against errant copy–pasters and editors of the code that don’t know what they’re doing. Perhaps at some point you’ll add or switch to reply.account.display_name, which probably can contain < and &.
3. The markup is produced by mixing static templating with user-provided input, and sanitisation is performed on the whole thing. It’s important when doing this sort of templating that each user-provided input be escaped or sanitised by itself in isolation, not as part of a whole that has been concatenated. Otherwise you can mess with the DOM tree accidentally or deliberately. Suppose, for example, that reply.content could contain `</div></div></div><img src=//ad.example alt="Legitimate-looking in-stream ad"><div class="mastodon-comment">…<div class="content">…<div class="mastodon-comment-content">…`. So this means:
• Apply attributes and text nodes to a real DOM (e.g. `img = new Image(); img.src = reply.account.avatar_static; avatar.append(img)`), or escape them in the HTML serialisation (e.g. `<img src="${reply.account.avatar_static.replace(/&/g, "&").replace(/"/g, """)}">`).
• Do HTML sanitisation on just the user input, e.g. DOMPurify.sanitize(reply.content).
A part of my problem with the code as written is that, purely from looking at the code, I can see that there may well be various security holes. I require knowledge of the backend also before I can judge whether there are security holes. Where possible, it’s best to write the code in such a way that you know that it’s safe, or that it’s not—try not to depend on subtle things like “the particular fields that we access happen to be unable to contain angle brackets, ampersands and quotes”, because they’re fragile.
Incidentally, it would also be more efficient to run DOMPurify with the RETURN_DOM_FRAGMENT option, and append that, rather than concatenating it to a string and setting innerHTML. Saves a pointless serialisation/deserialisation round trip, and avoids any possibility of new mXSS vulnerabilities that might be discovered in the future. (I don’t really understand why DOMPurify defaults to emitting a string. I can’t remember seeing a single non-demo use of DOMPurify where the string is preferable to a DOM fragment.)
—
† Though if the server sanitised arbitrary user-provided HTML/MathML/SVG, I probably don’t trust it as much as I trust DOMPurify, for things that end up in the DOM. There are some pretty crazy subtleties in things like HTML serialisation round-tripping of HTML, SVG and MathML content. There’s fun reading in the changelogs and security patches.