-1

I use Javascript to dynamically create a lot of elements. Divs, images, spans, etc.

Here is an example piece of code that my JS would run:

infoCell.innerHTML = "Submitted by " + "<a href='/user/" + this.poster + "'><img src='" + this.poster_avatar_src + "' class='avatarimg'>  <span style='color:blue'>" + this.poster + "</span> </a>in " + "<span style='color:blue; font-weight: 900;'><a href='/h/" + href + "'>" + this.topic + "</a></span>"

This was written early in my JS development, but now I realize that it can very quickly become very insecure, as almost all of the javascript variables being inserted into the HTML are written by the user with no limitations to character usage, etc.

How can I go through my javascript and change all of these so they still function, but without worrying about users inserting script into my site?

I am fine rewriting a lot but I would like to not do this again. I have about 90 innerHTML DOM modifications in my main JS file (typescript).

2 Answers2

1

you could try to use a combination of document.createElement and HTMLElement.append

an example for the first <a> tag:

function makeElem (tagname, properties) {
    let elem = document.createElement(tagname);
    for (const key in properties) {
        elem[key] = properties[key];
    }
    return elem;
}
infoCell.append("Submitted by ");
let a = makeElem("a", {href:'/user/"' + this.poster + '"'});
a.replaceChildren(makeElem("img", {'src':this.poster_avatar_src, 'className':'avatarimg'}), makeElem("span", {'textContent':this.poster,'style':'color:blue;'}));
infoCell.append(a);

this might not be the easiest but it should work, the reason for the "makeElem" function is purely convenience and you don't necessarily need it

Tristan
  • 31
  • 4
0

There are a few approaches.

One is to use a sanitizer to translate all of the dynamic values into properly escaped strings before interpolation - but you'd have to be sure you get it right, otherwise there could still be problems.

Another way is to construct the element structure, then insert the dynamic strings at the appropriate points, eg:

const cell = document.createElement('div');
cell.innerHTML = `
  Person info
  <div class="name"></div>
  <div class="age"></div>
`;
cell.querySelector('.name').textContent = name; // where name is dynamic
cell.querySelector('.age').textContent = age; // where age is dynamic

But this can be tedious if you have a lot of dynamic values to insert.

A third way (and one that I'd recommend for serious applications) is to use a framework to handle it for you. For example, in React, the above "cell" could be made like:

const Cell = ({ name, age }) => (
  <div>
    Person info
    <div class="name">{name}</div>
    <div class="age">{age}</div>
  </div>
);

It takes some learning and getting used to, but once you get going it's a lot easier to read and write than other approaches.

CertainPerformance
  • 356,069
  • 52
  • 309
  • 320
  • Thanks for the reply!! I think I understand, but I have a question. In your first example, couldn't a user still write their 'name' as an HTML string such as 'alert("test")'? I don't see how structuring would stop that. – mapleweekend Jan 17 '22 at 01:55
  • In the first approach, if the sanitizer is set up properly, any strings that attempt to make HTML tags will be escaped - eg `<` will turn into `&lt` - so there wouldn't be any way for input from the user to be executed as JavaScript. That's no guarantee that the input would make *sense*, of course, but it solves the security issue, *if* done right. – CertainPerformance Jan 17 '22 at 01:58
  • Got it, thank you! This is very helpful and has given me a few ideas on next steps! – mapleweekend Jan 17 '22 at 02:07
  • One last question. How about sanitizing on the backend? My app is taking data in from the user, parsing it, sending it to the backend, and then the backend is storing it in a MongoDB database to be later pulled through an API. If i just sanitize it before it hits the database, that seems the safest. Right? – mapleweekend Jan 17 '22 at 02:12
  • That's one possible approach, but "safest" is debatable. I'd consider assigning to the `.textContent` safest, because there's so little to it that there's not really any surface area for bugs. But the chance that there's a serious problem with any of the approaches is vanishingly unlikely regardless - in which case other factors (like readability and maintainability) become more important considerations. – CertainPerformance Jan 17 '22 at 02:20