Small Security Win - Preventing a Persisted XSS Vulnerability

My team recently caught and prevented a vulnerability from going into production. We had a last minute user story added to our sprint. It was fairly straight forward, render user entered data with line breaks onto a web page, so we took it on and assigned it to the developer that had some bandwidth.

He got his tasks done with a quick turn around, got his PR accepted, and handed off the story to QA. Everything seemed to be going smoothly. The QA tested the story, and sent it to product for UAT. Then we started merging for the scheduled sprint deployment.

In general, my team reviews code on a peer to peer basis. Even though I’m the lead, I don’t check and sign off on everything my team writes. I do try my best to read through the PRs that happen, but we don’t want to have rely on one member to sign off on everything, so as long as someone on the team approves, we generally move forward with the code. That changes when we merge for release. I’m still not a required approver for release merges, but I do scrutinize them more than during the sprint. So that’s when I spotted the use of dangerouslySetInnerHTML.

The developer used dangerouslySetInnerHTML to render the user content after swapping out all of the line breaks in the user content for <br />. Since this is user provided content, I immediately had alarm bells going off in my head when I saw this in a release merge. I also paused for a moment to think through how to get this fixed. I didn’t want to tell the developer bluntly: “this is a vulnerability fix it.” I wanted to make sure that I could nudge him the right direction, so I fired up a fiddler and tested out a couple of things that I thought might work before commenting on the PR. That way there would hopefully be some clear guidance on how to proceed.

With all this in mind, I suggested to him that we should break apart the user content, and then use jsx to render the <br /> into the DOM. He acknowledged the feedback, and said that he would give it a try. An hour or so later, he reached out to say that my suggestion didn’t work, which I found confusing because I had already tested a couple of things on my end. Everything should just click into place.

I go over to his desk and have him walk me through what he’s tried. I immediately see differences between my suggestion, and what he’s tried. I let him finish, ask him a few questions, and still see that we’re not on the same page yet. That’s when I suggest that we try doing a little paired programming, because I think there’s something that can still be done, and if I can help him navigate the problem then we should be able to get to the solution.

After a few minutes of pairing we have something like the following:

return (<div className="user-content-container">
    {userConent.split('\n')
      .map((line, i) =>
        <>
          <span key={i}>{line}</span></br>
        </>
      )}
  </div>);

This now works, and I end up fielding some follow up questions on why this was different than his previous attempts. He spends some more time testing his changes before we move on and merge the code with the vulnerability fixed.

I hope that this can serve as a case study of the boring side of security. The happy path of we had a mistake that we made because of human factors like a last minute request in our sprint, which we then fixed by having many eyes on the code, communicating well with each other, and using paired programming as a means of edification.

Note: As I was editing this article for publication, I found another approach that just uses CSS on stackoverflow.