XSS Related Typescript/Javascript Rules miss out vulnerable cases

Hello SonarQube Community.

We are using SonarQube Enterprise 9.9.1 deployed in a Docker container.

Currently I’m analysing SonarQubes ability to find DOM based XSS Vulnerabilities in Typescript Code. Especially in Angular Applications. There are already some rules that cover those types of vulnerabilities I am after:

My analysis show, that those rules from the “tssecurity” Repository try to prevent as much false positives as possible. Since they are of type vulnerability this is perfectly fine. But this also means, that the chance to miss out a actually vulnerable line of code is pretty high.

I created a quick test project using Angular, that contains a lot of those vulnerabilities, and scanned it with SonarQube. And I was really surprised, that not a single vulnerability was dedected by those rules. I started digging deeper and played around a bit with the code, and found out that this was due to the way I wrote my code. In Angular it is pretty common to encapsulate code into services and Inject it into Components or other Services. I used the same approach to structure my code. This was the reason, why the vulnerabilities where not dedected.


Lets have a look at a few examples. All of them use the following DataService that has a single “location” method. This method accesses the location hash, that is controllable by the user. And could lead to XSS, if used in a unsafe context.

    providedIn: 'root',
export class DataService {
    location(): string {
        return location.hash.substring(1);

S5334 setTimeout, setInterval

  selector: "app-root",
  templateUrl: "./app.component.html",
  styleUrls: ["./app.component.scss"],
export class AppComponent implements OnInit, AfterViewInit {
  constructor(public dataService: DataService) {}

  ngOnInit(): void {
    setTimeout(this.dataService.location(), 1000); // not detected by S5334
    setTimeout(location.hash.substring(1), 1000); // detected by S5334

Both setTimeout are invoced with the exact same string parameter. But the rule does not detect the first call. Because it does not treat this.dataService.location() as a unsafe source.

S5696 innerHTML

  selector: "app-root",
  templateUrl: "./app.component.html",
  styleUrls: ["./app.component.scss"],
export class AppComponent implements OnInit, AfterViewInit {
  @ViewChild('playground', { static: true })
  playground?: ElementRef<HTMLDivElement>;
  constructor(public dataService: DataService) {}

  ngAfterViewInit(): void {
    const div1 = document.createElement("div");
    div1.innerHTML = this.dataService.location(); // Not detected by S5696 

    const div2 = document.createElement("div");
    div2.innerHTML = location.hash.substring(1); // Detected by S5696

    const element: HTMLDivElement = this.playground.nativeElement;
    element.innerHTML = location.hash.substring(1); // Not detected by S5696

Same as before. div1.innerHTML is not detected because the dataService is not treated as unsafe source. Also element.innerHTML is not dtected. The rule might not realise, that we call innerHTML on an actual DOM element as it comes from an Angular @ViewChild.


I would like to have all cases of dangerous calls or assignments to be reported by SonarQube. Regardless of the values assigned to them (With the exception of literal strings. Those should be fine). I think Security Hotspots are the perfect fit for those kind of vulnerabilities. A Developer has to review the code, and decide if there is a problem or not. Especially when using Angular or other modern frameworks, there should not be the need to use those unsafe constructs very often. And if you do, there is probalby something wrong with the code.

The examples above are very simple. But in a real live scenario, the data might be loaded from a server, and then be passed to a unsafe sink like innerHTML. No tool would be able to say, if the data returned from the server contains user controlled data or not. In that case, a Security Hotspot is the perfect solution.

I would like to hear your opinion on this topic.
Is it already knwon, that the rules have those limitation?
Are there any plans to extend those rules?
Or maybe the exiting Rules that find vulnerabilities for certain, and Hotspot generating Rules can coexist?

I would also be happy to contribute some ESLint rules to the SonarJS repository when desired.

1 Like

Hello Daniel,

Welcome to the community and thanks a lot for the detailed report!

It does not come as a surprise that we do not detect these issues, we are aware of limitations in that regard. Due to the nature of JavaScript, it is difficult to properly follow the tainted data. With our current approach, we have to model all the pieces in-between by hand, e.g., what value dataService contains. If a piece is missing, no issue is raised. We are currently in the process of researching more effective ways how to do taint analysis for JavaScript, but it is a long process.

As for the hotspots, it is worth a consideration. I am just afraid it could generate a lot of noise for some people. This is something that we try to prevent because if there is too much noise, developers stop looking at the results. So it would be desirable to only report real vulnerabilities. I would say, let’s wait and see what our new approaches will bring and if the problem still exists, we can start looking into just flagging certain function calls or assignments.

As an alternative for now, maybe the generic issue import format could be of interest to you. A “grep” converted to this format should pretty much give the desired results, unless the code is obfuscated.

Thanks for your quick reply.

Good to hear, you are researching more efficient ways to find thos kind of vulnerabilities. :+1:

Regarding the lot of noise for some people. You are right. This is something to keep in mind. If this is split into multiple rules:

  • One that finds locations that are certainly vulnerabilities (like the rule works now)
  • And another rule that creates hotspots to review.
    Then people can always decide to disable the rule that generates a lot of noise. Or a rule could be made optional, and one can enable it if they like.

In my opinion, such a rule might not produce that much noise, that is not worth reviewing it.

If you use a modern frontend framework, usage of such API is not the norm. If you make heavy use of those APIs, you certainly do something wrong and you might reconsider your coding style instead of complaining about a lot of noise in sonarqube :grin:

And if you do not use any frameworks at all, and work with the plain DOM APIs, it is probably a good thing to be pointed to the places in your code, that needs a closer look. You can easily shoot yourself in the foot, if you have 200 innerHTML calls with dynamically built strings, that are not passed through a sanitizer.

Maybe realxing the taint analysis to find more locations, and instead check that the data was passed into a sanitizing function beforehand, would make the rule a lot easier to maintain. Sanitizing data before passing it to a DOM sink is best practice anyway.

But thanks for taking the time to answer my question. I’m looking forward to see what your new approach is capable of, and will have a closer look once it is available.