Your popup.js still isn't sanitized properly. It is possible to run JavaScript code by injecting a custom href string


(Ahmad Raza) #1

I submitted an add-on but reviewer raised this issue, can anyone please help me?


(Martin Giger) #2

If only an href is the issue you could just check for it to start with a trusted protocol.


(erosman) #3

Which addon? Please post the addon URL so that it can be checked.


(Ahmad Raza) #4

it was a USA TODAY link so it was a trusted source, btw i sanitized that link too, hopefully no issue will be raised now…


(Ahmad Raza) #5

https://addons.mozilla.org/en-US/firefox/addon/usa-today-news/
this is the link…


(erosman) #6

Problem is here:
NewNode1.setAttribute('href', dec_link);

This is what I suggest in my reviews:

Inserting un-sanitized remote content is a security problem. For example, JavaScript can be passed as href/src/on** etc.
Addon should ensure such strings are not-executable (and not javascript:somefunction).
In case of href/src, a simple startsWith(‘http’) would be sufficient for this purpose.

There are other issues in that add-on.

It says “USA TODAY- News” but it gets its data from a 3rd party site.

Please mention in the addon’s description that the addon uses/requires a 3rd party site or application (newsapi.org) for its operation.

Also…

Suggestion: With small/minor changes to code using pure JavaScript, addon would no longer need additional libraries (ie JQuery etc) which improves its performance and efficiency.


(Ahmad Raza) #7

bro the link in dec_link starts with https://, i do not understand where else should i add http…


(erosman) #8

How do we know it starts with http?
How does the addon code know it starts with http?

The only way for the code to know whether it starts with http is to check if it starts with http, as suggested in the review.

fetch(req).then(....

for(var i=0; i<data.articles.length; i++)
{ ...
let news_link = data.articles[i].url;
let enc_link = encodeURIComponent(news_link); // <- this has no use
let dec_link = decodeURIComponent(enc_link); // <- this has no use
NewNode1.setAttribute('href', dec_link);

(Ahmad Raza) #9

ooh i should use rejex for that right?


(erosman) #10

Not needed… please read what I have posted …


(Ahmad Raza) #11

thanks you so much brother…