search

leizongmin / js-xss

Sanitize untrusted HTML (to prevent XSS) with a configuration specified by a Whitelist
http://jsxss.com
Other
5.21k stars 630 forks source link

Confusing variable assignment - Eslint should be configured for this project #251

Open ctaschereau opened 2 years ago

ctaschereau commented 2 years ago

I was tracking down an issue I had and was reading the code when I saw this :

https://github.com/leizongmin/js-xss/blob/daa471e560d967a5bcbe1a00be40a8ffe91405b3/lib/xss.js#L133-L140

Even my IDE was confused by this and we both thought (upon first glance) that the line 137 was shadowing the one on 135 but that was not the case since this project is using "var" instead of "let".

Since the var keyword hoists the declaration to the top of the function, the line 137 does not in fact shadow the previous one, but that is very confusing when reading the code.

I think that the easiest fix would be to simply remove that var keyword on line 137. A better fix would also include converting this project to use the let keyword. Eventually, setuping eslint correctly would have caught something like this.

leizongmin commented 2 years ago

Hi @ctaschereau , I agree with you. I think this project may need to be rewritten using ES6 syntax in the future. I'll do it if I have time.

lumburr commented 2 years ago

I am very willing to contribute to the js-xss. I think it's possible to add an eslint first to improve code readability. I'm also more than willing to help if you want to rewrite in es6/typescript.