Fortify Scan finds a critical vulnerability

[AleksueiR]

Describe the bug
A Fortify Scan finds a critical Cross-Site Scripting vulnerability in Axios here:

https://github.com/axios/axios/blob/master/lib/helpers/isURLSameOrigin.js#L30

urlParsingNode.setAttribute('href', href);

The method resolveURL() sends unvalidated data to a web browser, which can result in the browser executing malicious code.

This issue has been reported previously, but then it was closed as 0.19.0 was supposed to provide a fix.

We ran a Fortify scan on 0.19.0 and got the same error; the source code still contains the problematic line above and the issue remains.

[mkotsollaris]

This seems to still be an issue according to snyk.io?

[pimotte]

I don't think #2451 fixes this, as mentioned by this comment

From what I see so far, the url that gets here is already url-encoded, so there should be no problems, but doing a pass of url-encoding should fix the issue for sure, right?

Edit: On second thought, is this even vulnerable? https://security.stackexchange.com/questions/186839/xss-breaking-out-of-json-parse-and-href-attribute

[AleksueiR]

@pimotte might not be a real vulnerability, but it trips the scanner and then everyone wants an explanation why we are using a library with a critical vulnerability in it. I would prefer this fixed even if it's a false positive, so Fortify scans come out clean.

[dominykas]

This is not exploitable - the element never ends up in the DOM.

This should be raised as a bug in Fortify, I think.

[benjifin]

Just a heads up from Snyk - thanks to @dominykas pulling us in here we've reviewed the vulnerability and determined it really is a false positive on Fortify's account - and we are going to be removing the vulnerability from our DB. Apologies for the noise on our side, and please let us know if we can be of help talking to people at Fortify to get the issues cleared up if you all opt to not release any changes on account of this scan in the end.

[yasuf]

I'm almost sure the vulnerability is still there, @benjifin would you mind taking a look at the PR I just opened? The previous code to check for an XSS attack wasn't throwing an error or preventing the attack and the test wasn't checking for that either. Does someone have the ability to run the fortify analysis by any chance?

[snoopysecurity]

Hey @yasuf, I also work for @snyk. @benjifin and I had a look at your PR, I agree with you amending the logic of the isValidXss function since it didn't throw an error when an potential malicious XSS payload was found. However looking at code behind isValidXss itself, this is not the correct way to prevent XSS since isValidXss is just a simple blacklist . I ll write some brief notes on your PR itself.

But in general, I agree with @dominykas's comment regarding this vulnerability, the element the user input is inserted to is never rendered in the DOM hence we removed this issue from Snyk VulnDB.

Hope this helps.

[yasuf]

this issue can now be closed @AleksueiR 🙏 would be nice if we can confirm the vulnerability doesn't give the false positive

[AleksueiR]

If there is a new release, I can run it through a Fortify scan.

[chinesedfan]

we've reviewed the vulnerability and determined it really is a false positive on Fortify's account

As @benjifin says, we have released 0.19.2 to revert the overkill patch in 0.19.1.

[AleksueiR]

We ran a Fortify scan and there were no issue. Thanks! 👍

[JyothiYamajala]

@snoopysecurity @dominykas Can you help me understand how did you come to a conclusion that it is not an issue. That is that it is not exploitable as the element never ends up in DOM?