Is teaching DOM manipulation using innerHTML a worthwhile tradeoff?

[40thieves]

As discussed during the last couple of Tech Ed syllabus team meetings, I have noticed that we have adopted the use of innerHTML when teaching DOM manipulation - at least when it comes to "child elements", as seen in this section of JS3 Week 1:

const filmCard = document.createElement("section");
filmCard.innerHTML = `
  <h3>${film.title}</h3>
`;

Unfortunately innerHTML is vulnerable to XSS attacks, since the browser will treat the assigned value literally as "normal" HTML, giving it the same power as code written by the web developer. This can be abused by an attacker by crafting values that will execute JS in the victim's browser, potentially stealing cookies or other malicious actions. Using the example from the JS3 Week 1 code snippet above, if film.title was controllable by a user, you could set the value to something like: <script>alert('haxxored')</script>. If the snippet above is then run, you would see the alert dialog popup as if the author of the page had written that code.

The safe alternative is to rewrite the snippet above as something like this:

const filmCard = document.createElement("section");

const filmCardTitle = document.createElement("h3");
filmCardTitle.textContent = file.title;

filmCard.append(filmCardTitle);

Here, I'm constructing each individual element as a DOM object, then appending it together. Note that textContent (or potentially innerText) will not treat the value as HTML and so will not execute any JS within it.

The major downside of this approach is that it is unfortunately significantly less readable than the innerHTML version. This is more obvious when building multiple nesting DOM elements:

const filmCard = document.createElement("section");
const filmCardRelease = document.createElement("h2");

const filmCardReleaseSpan = document.createElement("span");
filmCardReleaseSpan.textContent = "Released on: ";
const filmCardReleaseDate = document.createElement("time");
filmCardReleaseDate.textContent = film.release;

filmCardRelease.append(filmCardReleaseSpan, filmCardReleaseDate);

filmCard.append(filmCardRelease);

---

This presents a difficult tradeoff: do we teach a technique that is considerably easier to understand but insecure? Or the opposite? Or an alternative option?

[40thieves]

One possible alternative approach would be to teach lean into the problem by showing the innerHTML technique initially, but then demonstrating the security problem similar to how I've laid it out above to explicitly get trainees to understand why this is a poor technique.

This has the benefit of teaching them about security, and XSS which is one of the OWASP Top 10. Like accessibility knowledge, security is potentially a valuable differentiator in the employability market. (Although I should note that security is a big topic, and basic knowledge of XSS probably isn't enough to cut it).

On the flip side, I think there is a risk that we would just confuse trainees and they would learn neither approach.

[40thieves]

From my personal experience, the CTO of the company I work for is extremely conscious of security (probably rightly so!) and is asked about in every engineering interview that we run.

I have a feeling that if he got wind that we were explicitly teaching innerHTML without also teaching XSS/how to avoid it, then he would not look favourably on our graduates.

[sztupy]

Is it worth teaching DOM manipulation as a basic feature at all if we end up teaching them React in the end? Lot of trainees had issues with this part at least in my group, and later got relieved that using React this is all taken care of, and by the time they graduated have likely forgotten everything about this already. There was only one project that had used non-react DOM manipulation and that was done in a single component as well.

[SallyMcGrath]

I think if you work in FE web you need to eventually understand the DOM to be any good. I don't think we want to make them utterly dependent on React. There's still a whole heap of non-React out there, and likely to be a lot more in the future.

[SallyMcGrath]

I think the createElement stuff is fairly unreadable and encourages invalid/inaccessible HTML. A suggestion here would be to use normal HTML templates? It's still pretty verbose but it's closer to frameworks, where they typically populate a template with data. Thinking about the TV Show project:

<template id="episode-card">
          <li class="card">
            <h3 class="card__title">Episode Title and Number Code</h3>
            <p class="card__summary"></p>
            <img
              src=""
              alt="TV Show"
              class="card__image"
              width="250"
              height="140"
            />
          </li>
        </template>

const episodeCard = (episode, template) => {
  const template = document.getElementById(template);
  const card = template.content.cloneNode(true);
  card.querySelector(".card").setAttribute("id", episode.id);
  card.querySelector(".card__image").setAttribute("src", episode.image.medium);
  card.querySelector(".card__title").textContent = episode.name
  card.querySelector(".card__summary").append(episode.summary);
  return card;
};

or even I guess if we want to take it closer to a react component we could show destructuring

const episodeCard = (template, { id, image: { medium }, name, summary }) => {
  const card = document.getElementById(template).content.cloneNode(true);

  card.querySelector(".card").setAttribute("id", id);
  card.querySelector(".card__image").setAttribute("src", medium);
  card.querySelector(".card__title").textContent = name;
  card.querySelector(".card__summary").append(summary);

  return card;
};

I don't think it's a solution to just pretend innerHTML doesn't exist because ChatGPT will suggest it to them anyway. I just tried some of the JS2/3 exercises asking it to help me work through the problems and it actually wouldn't STOP writing innerHTML even when I forbade it. So maybe two things: more readable code samples using templates, and a brief workshop/project on security.

InnerHTML is only a problem when you're not in control of the content and it hasn't passed through some kind of sanitisation, so I think probably that insight is the useful part to develop. If only they would actually release that Sanitizer API!

[SallyMcGrath]

To more closely explain on the JS3 stuff

const filmTemplate = document.getElementById("filmTemplate");

function createFilmCard({title, times, certificate, duration}) {
  const card = filmTemplate.content.cloneNode(true);

  card.querySelector(".title").textContent = title;
  card.querySelector(".times").textContent = times.join(", "); 
  card.querySelector(".certificate").textContent = certificate;
  card.querySelector(".duration").textContent = duration;

  return card; 
}

const films = [
  {
    title: "Killing of Flower Moon",
    times: ["15:35"],
    certificate: "15", 
    duration: 112
  },
  {
    title: "Typist Artist Pirate King", 
    times: ["15:00", "20:00"],
    certificate: "12A",
    duration: 108
  }
];

films.map(createFilmCard) 
.forEach(card => {
  document.body.append(card)
});

[illicitonion]

One thing I'll add of JS3 context: The TV Maze API we use for the TV show project returns literal HTML as a string which we expect our trainees to display in the page (example, see these disappointing lines in my sample implementation).

I would love for our trainees to be able to identify this as a problem, and consider approaches to mitigate it; whatever approach we pick, maybe this could be useful as a case study?

[SallyMcGrath]

Oh interesting, That's not how I solved it. I didn't even consider it, actually! I stripped the p tags. This is likely because it's such a common issue when working with CMS, and most FE templating languages have a strip tags method

https://twig.symfony.com/doc/3.x/filters/striptags.html
https://shopify.github.io/liquid/filters/strip_html/
https://gohugo.io/functions/transform/plainify/

etc. So I just wrote a strip tags function and passed it through.

[SallyMcGrath]

Update on this

It's not a good tradeoff. We have solved the problem another way. JS3 has been rewritten!